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

fix: Duplicate classes and invalid aria props on LanguageSelector #2893

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

Conversation

sawyerh
Copy link
Contributor

@sawyerh sawyerh commented Apr 18, 2024

Summary

This fixes a few issues with the LanguageSelector implementation:

  1. The usa-language-container class was being applied to both the parent div and the inner button, which causes layout issues due to the button receiving unexpected margins and relative positioning
  2. [fix] Language selector for two languages has invalid ARIA attribute #2809

An assumption I am making in this PR is that the intent of the className prop is to apply custom styling to the button, which is how it's demonstrated in Storybook.

How To Test

Storybook

Screenshots (optional)

CleanShot 2024-04-18 at 15 08 34@2x

@sawyerh sawyerh requested a review from a team as a code owner April 18, 2024 22:08
@@ -14,6 +14,9 @@ export type LanguageSelectorProps = {
label?: string
langs: LanguageDefinition[]
small?: boolean
/**
* Custom classes for the call-to-action
*/
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 shows in Storybook:

CleanShot 2024-04-18 at 15 09 03@2x

@@ -27,8 +25,6 @@ export const LanguageSelectorButton = ({
<button
data-testid="languageSelectorButton"
className={classes}
aria-expanded={isOpen}
aria-controls="language-options"
Copy link
Contributor Author

@sawyerh sawyerh Apr 18, 2024

Choose a reason for hiding this comment

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

ℹ️ These are now passed in as buttonProps from LanguageSelectorDropdown since that's the only context they need applied

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.

None yet

1 participant