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

Remove ariaLabel prop and use same translation close for dialog #2097

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

bryancunningham-okta
Copy link
Contributor

@bryancunningham-okta bryancunningham-okta commented Jan 23, 2024

OKTA-683644

Summary

Testing & Screenshots

  • I have confirmed this change with my designer and the Odyssey Design Team.

@@ -111,7 +116,7 @@ const Dialog = ({
<DialogTitle translate={translate}>
{title}
<Button
ariaLabel={ariaLabel}
ariaLabel={t("close.text")}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to fall back to the supplied one for now?
Also, if someone uses this prop, can we give them a warning / error at build time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This translation already exists for Toast so, I just used the same one. Not sure about warning at build time. No strong feelings either way

Copy link
Contributor

Choose a reason for hiding this comment

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

I just wonder if we should warn users about this, if we detect they set the prop. We're pretty much dropping the value on the floor now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrispulsinelli-okta I'm fine with warning users, if that is what we want to do. How do I best accomplish that?

Copy link
Contributor

@chrispulsinelli-okta chrispulsinelli-okta left a comment

Choose a reason for hiding this comment

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

I'm good with the changes and leave it up to you if you want to leave a warning for users if they set the prop we deprecated.

Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

Other than one comment removal, this is approved 👍

@bryancunningham-okta bryancunningham-okta merged commit facfbce into main Feb 2, 2024
1 check passed
@bryancunningham-okta bryancunningham-okta deleted the bc/OKTA-683644/dialog-close-translation branch February 2, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants