Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(dialog-wrapper): add dismiss-label attribute for the close button's label #4154

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

susmithayenugula
Copy link
Contributor

@susmithayenugula susmithayenugula commented Mar 7, 2024

dialog-wrapper has several labels for customization. This PR introduces a new attribute dismiss-label for dialog-wrapper and dialog. It allows the string passed in to be used as the label for dialog-wrapper and dialog.

Description

  • Added new property dismissLabel and attribute dismiss-label to dialog-wrapper and dialog. Default is ``.
  • dismiss-label will be used as the sp-close-button's label prop.
  • Updated all related dialog-wrapper and dialog stories to pass in dismiss-label = 'Close'.
  • Updated 2 unit tests to check that the dismiss-label set in the stories trickles down to the aria-label of the components.
  • Updated dialog-wrapper readme with new dismiss-label.

Related issue(s)

fixes #3375

Motivation and context

#1975

How has this been tested?

DialogWrapper

  1. Navigate to storybook
  2. In the components sidebar, select and expand Dialog Wrapper.
  3. Select any story that uses a dismissable dialog. For e.g., Wrapper Dismissable.
  4. In DevTools, use the select tool to inspect the close ( x ) button.
  5. Verify that the sp-close-button has label = 'Close'
  6. Verify the parent sp-dialog and sp-dialog-wrapper has dismiss-label = 'Close'

Dialog

  1. Navigate to storybook
  2. In the components sidebar, select and expand Dialog.
  3. Select any story that uses a dismissable dialog. For e.g., Dismissable.
  4. In DevTools, use the select tool to inspect the close ( x ) button.
  5. Verify that the sp-close-button has label = 'Close'
  6. Verify the parent sp-dialog has dismiss-label = 'Close'

Screenshots (if appropriate)

Dialog Wrapper

DialogWrapper DialogWrapper - property, attribute

Dialog

Dialog Dialog - Property Dialog - Attribute

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Copy link

github-actions bot commented Mar 7, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.97 0.98 0.98
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 229.288 kB 217.614 kB 🏆 227.754 kB
Scripts 61.146 kB 54.833 kB 54.503 kB 🏆
Stylesheet 35.548 kB 30.884 kB 🏆 41.411 kB
Document 5.882 kB 5.185 kB 5.128 kB 🏆
Third Party 126.712 kB 126.712 kB 126.712 kB

Request Count

Category Latest Main Branch
Total 43 43 43
Scripts 35 35 35
Stylesheet 5 5 5
Document 1 1 1
Third Party 2 2 2

Copy link

github-actions bot commented Mar 7, 2024

Tachometer results

Chrome

dialog permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 507 kB 72.73ms - 74.99ms - unsure 🔍
-6% - +0%
-4.44ms - +0.13ms
branch 499 kB 74.03ms - 78.00ms unsure 🔍
-0% - +6%
-0.13ms - +4.44ms
-
Firefox

dialog permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 507 kB 110.25ms - 117.43ms - unsure 🔍
-5% - +3%
-5.69ms - +3.85ms
branch 499 kB 111.62ms - 117.90ms unsure 🔍
-3% - +5%
-3.85ms - +5.69ms
-

@@ -61,6 +61,9 @@ export class Dialog extends ObserveSlotPresence(AlertDialog, [
@property({ type: Boolean, reflect: true })
public dismissable = false;

@property({ type: String, reflect: true, attribute: 'dismiss-label' })
public dismissLabel = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please default this to Close to reduce default effort required to consume this pattern.

This should also prevent the need to alter the story files, unless you'd like to add one that leverages this for an internationalized delivery.

@Westbrook Westbrook force-pushed the yenugula/issue-3375 branch from 4782663 to ce577c4 Compare March 20, 2024 21:29
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

LGTM!

@Westbrook Westbrook changed the title feat(dialog-wrapper): add dismiss-label attribute for the close button's label fix(dialog-wrapper): add dismiss-label attribute for the close button's label Mar 20, 2024
@Westbrook Westbrook merged commit c450a09 into main Mar 20, 2024
50 checks passed
@Westbrook Westbrook deleted the yenugula/issue-3375 branch March 20, 2024 21:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat][a11y]: sp-dialog-wrapper, add dismissable-label to provide a label for the close button
2 participants