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

feat: Adding aria label to property filter token operator button #491

Merged
merged 10 commits into from
Dec 1, 2022

Conversation

varghvi
Copy link
Contributor

@varghvi varghvi commented Nov 16, 2022

Adding operatorAriaLabel property to I18nStrings to set aria-label to filter token operator.

AWSUI-19118

[Include a summary of the changes and the related issue.]

[Also include relevant motivation and context.]

How has this been tested?

[How did you test to verify your changes?]

[How can reviewers test these changes efficiently?]

[Check for unexpected visual regressions, see CONTRIBUTING.md for details.]

Documentation changes

[Do the changes include any API documentation changes?]

  • Yes, this change contains documentation changes.
  • No.

Related Links

[Attach any related links/pull request for this change]

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@varghvi varghvi requested a review from a team as a code owner November 16, 2022 20:19
@varghvi varghvi requested review from timogasda and removed request for a team November 16, 2022 20:19
@varghvi varghvi changed the title adding aria label to property filter token operator button Fix: Adding aria label to property filter token operator button Nov 16, 2022
@varghvi varghvi changed the title Fix: Adding aria label to property filter token operator button AWSUI-19118: Adding aria label to property filter token operator button Nov 16, 2022
@avinashbot avinashbot requested review from YueyingLu and avinashbot and removed request for timogasda November 17, 2022 14:32
@@ -155,6 +155,7 @@ const InternalSelect = React.forwardRef(
{...formFieldContext}
controlId={controlId}
ariaLabelledby={joinStrings(formFieldContext.ariaLabelledby, selectAriaLabelId)}
ariaLabel={ariaLabel}
Copy link
Member

Choose a reason for hiding this comment

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

No need to add ariaLabel in Select because Select does it itself already. You can remove this line and the change in select/parts.

<ScreenreaderOnly id={selectAriaLabelId}>{ariaLabel}</ScreenreaderOnly>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YueyingLu this is fixed now.

@@ -215,6 +215,7 @@ export namespace PropertyFilterProps {
tokenLimitShowMore?: string;
tokenLimitShowFewer?: string;
clearFiltersText: string;
operatorAriaLabel?: string;
Copy link
Member

Choose a reason for hiding this comment

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

operatorAriaLabel to me is not very clear, which operator it refers to as there are many operators for different uses in PropertyFilter. I suggest more specific naming like tokenOperatorAriaLabel. @avinashbot wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it made sense to me, so went ahead and fixed, @avinashbot, let me know if you see any issues.

@YueyingLu

@YueyingLu YueyingLu changed the title AWSUI-19118: Adding aria label to property filter token operator button feat: Adding aria label to property filter token operator button Nov 17, 2022
src/select/parts/trigger.tsx Outdated Show resolved Hide resolved
@YueyingLu
Copy link
Member

Hi @varghvi
We had a project team discussion and agreed on tokenOperatorAriaLabel.
Because it has api change, we need to update the documenter snapshot: run npm run build then npx jest -c jest.unit.config.js src/__tests__/documenter.test.ts. It should resolve the test failure.

@YueyingLu YueyingLu self-requested a review November 24, 2022 10:42
@varghvi varghvi closed this Nov 25, 2022
@varghvi varghvi reopened this Nov 25, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Base: 92.38% // Head: 92.38% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (2567e9d) compared to base (cc4db96).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #491   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         569      569           
  Lines       16218    16219    +1     
  Branches     4433     4433           
=======================================
+ Hits        14983    14984    +1     
  Misses       1151     1151           
  Partials       84       84           
Impacted Files Coverage Δ
src/property-filter/token.tsx 100.00% <ø> (ø)
src/internal/components/filtering-token/index.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@varghvi varghvi closed this Nov 29, 2022
@varghvi varghvi reopened this Nov 29, 2022
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