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

chore(action): add new string for accessible indicator label #10360

Merged
merged 5 commits into from
Sep 20, 2024

Conversation

driskull
Copy link
Member

@driskull driskull commented Sep 19, 2024

Related Issue: #9071

Summary

  • check in new string

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Sep 19, 2024
@driskull driskull marked this pull request as ready for review September 20, 2024 15:44
@driskull driskull added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 20, 2024
: "";
const labelFallback = label || text || "";
const ariaLabel = indicator
? messages.indicatorLabel?.replace("{label}", labelFallback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we can start using new message string before translations are back. This will be remove aria-label for all non-english locales.

Ex:

 <calcite-action icon="banana" scale="l" label="test" indicator text="test2" lang="bs"></calcite-action>

☝️ wouldn't add aria-label to button in shadowRoot.

Alternate: we can request a generate build which return english strings for all locales or wait until the translations are back before start using indicatorLabel string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we just add the string to the messages for now then and move this issue to another milestone? What would you recommend?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we need a better process for issues with string changes to get the strings in first. Separate the issues.

Copy link
Contributor

@anveshmekala anveshmekala Sep 20, 2024

Choose a reason for hiding this comment

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

Should we just add the string to the messages for now then and move this issue to another milestone? What would you recommend?

Yup that works. Lets add separate PR & issue for message string and it as blocker for #9071

Second creating separate issue for strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update the PR to just check in the string. Once the string is localized we can use it.

const labelFallback = label || text || "";
    const ariaLabel = indicator
      ? messages.indicatorLabel?.replace("{label}", labelFallback)
      : labelFallback;

Copy link
Contributor

@anveshmekala anveshmekala 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 the comment LGTM!

@anveshmekala
Copy link
Contributor

anveshmekala commented Sep 20, 2024

Sidebar & Unrelated to issue:

@geospatialem could you please review if we need Indicator present message in parenthesis for accessible label. Can handle in separate issue if it is not required.

@driskull driskull changed the title fix(action): create localized template for aria-label chore(action): add new string for accessible indicator label Sep 20, 2024
@driskull driskull added i18n-l10n issues dealing with internationalization/localization and removed bug Bug reports for broken functionality. Issues should include a reproduction of the bug. labels Sep 20, 2024
Copy link
Contributor

@anveshmekala anveshmekala left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

:shipit:

@driskull driskull merged commit 06c96e9 into dev Sep 20, 2024
18 checks passed
@driskull driskull deleted the dris0000/action-indicator-text branch September 20, 2024 21:13
benelan added a commit that referenced this pull request Sep 20, 2024
* origin/dev:
  chore(action): add new string for accessible indicator label (#10360)
benelan added a commit that referenced this pull request Sep 30, 2024
…estone-estimates

* origin/dev: (29 commits)
  fix(input-time-zone): fix region mode labeling and value mapping (#10345)
  fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264)
  refactor(components): reduce post-migration TypeScript errors (#10356)
  refactor: do not use Host in functional components (#10352)
  refactor(tests): reduce TypeScript errors (#10344)
  feat(alert): add component tokens (#10218)
  fix(card): properly handle slotted elements (#10378)
  refactor(panel): remove duplicate tailwind class (#10379)
  feat(popover, action): add component tokens (#10253)
  chore(t9n): add translations (#10339)
  feat: add 3d building, 3d building parameter, divide, parcel, spaces, spaces parameter, square brackets x, n variable, zoning parameter (#10373)
  build: update browserslist db (#10370)
  ci: resolve husky pre-push and pre-commit errors (#10365)
  docs: update component READMEs (#10371)
  feat(text-area): add component tokens (#10343)
  fix(action): prefer `disabled` in favor of `aria-disabled` (#10367)
  docs(a11y): add reference to a11y guidance to issue template (#10372)
  chore(action): add new string for accessible indicator label (#10360)
  feat(chip): add component tokens (#10179)
  feat(avatar): add component tokens (#10280)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-l10n issues dealing with internationalization/localization skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants