Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moved description into dl tag in message list component #3465

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SriHV
Copy link
Contributor

@SriHV SriHV commented Dec 24, 2024

What is the context of this PR?

204

Moved description into dl tag in message list component.

To achieve this new param bodyLabel was added and ariaLabelMsg param was deprecated.

As per the deprecation implementation process available in confluence
(DEPRECATED) is added to the ariaLabelMsg param and NEW is added to the bodyLabel param in message list documentation.

How to review this PR

Inspect example-message-list.html in send and reply message patterns and check that the description is inside the dl tag.

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

Copy link

netlify bot commented Dec 24, 2024

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit a7cb607
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/678116b615d4840008a046ef
😎 Deploy Preview https://deploy-preview-3465--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SriHV SriHV self-assigned this Dec 24, 2024
@SriHV SriHV added the Bug Something isn't working label Dec 24, 2024
@SriHV SriHV marked this pull request as ready for review December 24, 2024 17:24
@@ -15,10 +15,11 @@
<dd class="ons-message-item__metadata-value ons-message-item__metadata-value--date ons-u-fs-s">
{{ message.dateText }}
</dd>
<dt class="ons-message-item__metadata-term ons-message-item__metadata-term--body ons-u-d-no">{{ params.body }}:</dt>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params.body isn't a param thats being used or passed in the example or documented and you have removed the ariaLabelMsg param. This would be a breaking change, to avoid the breaking change for now we should probably use the ariaLabelMsg param for this and then record this as a breaking change that we would want to make in the next breaking release, check our confluence for the process for that. Because of this we should also default it to "Message text" like ariaLabelMsg already is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed we would keep the ariaLabelMsg param but introduce a new param params.bodyLabel, bodyLabel would set the new message label but if that doesn't exist it would fall back to use ariaLabelMsg

src/components/message-list/_macro.njk Outdated Show resolved Hide resolved
src/components/message-list/_macro.njk Outdated Show resolved Hide resolved
src/components/message-list/_macro.spec.js Show resolved Hide resolved
@rmccar
Copy link
Contributor

rmccar commented Jan 10, 2025

As this is a deprecation we need to follow the process outlined in the "Deprecation Implementation and Release Process" in confluence we have set out. You have added the "(DEPRECATED)" tag but we need to follow the other parts of the process

@SriHV
Copy link
Contributor Author

SriHV commented Jan 10, 2025

As this is a deprecation we need to follow the process outlined in the "Deprecation Implementation and Release Process" in confluence we have set out. You have added the "(DEPRECATED)" tag but we need to follow the other parts of the process

I had doubt on this one. I will add the rest now now

@@ -39,7 +41,6 @@ const EXAMPLE_MESSAGE_LIST = {
...EXAMPLE_MESSAGE_LIST_MINIMAL,
ariaLabel: 'Message list for ONS Business Surveys',
ariaLabelMetaData: 'Message information',
ariaLabelMsg: 'Message preview',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we still support this param we still need to test it. We will need to add an example in where this is set instead of bodyLabel and a test similar to the new test you have added

@@ -20,6 +20,7 @@ const EXAMPLE_MESSAGE_LIST_MINIMAL = {
},
fromText: 'Example Sender 1',
dateText: 'Tue 4 Jul 2020 at 7:47',
bodyLabel: 'Body',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This param shouldn't be at the message level, it should be at the top param level like the other label params

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants