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

add strings prop to PresenceBadge and update demos to support new default strings #22775

Closed

Conversation

micahgodbolt
Copy link
Member

fixes #21137

Adds strings prop to PresenceBadge to allow for default and custom strings. Updated badge to not hide badges by default, and updated AvatarBadge story to not have custom aria-labels.

@micahgodbolt micahgodbolt requested review from a team, behowell and khmakoto as code owners May 2, 2022 23:00
@micahgodbolt micahgodbolt requested a review from smhigley May 2, 2022 23:06
@codesandbox-ci
Copy link

codesandbox-ci bot commented May 2, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 82ef050:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

📊 Bundle size report

🤖 This report was generated against ace1c2193cbff237cb4cda006dad4ed7f8eb1e74

* Link: Adding hrefs to all non-button stories.

* Update packages/react-components/react-link/src/stories/LinkInline.stories.tsx

Co-authored-by: Micah Godbolt <micahgodbolt@gmail.com>

* Update packages/react-components/react-link/src/stories/LinkInline.stories.tsx

Co-authored-by: Micah Godbolt <micahgodbolt@gmail.com>

Co-authored-by: KHMakoto <humberto_makoto@hotmail.com>
Co-authored-by: Micah Godbolt <micahgodbolt@gmail.com>
@size-auditor
Copy link

size-auditor bot commented May 2, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: a18138c9bb8b8711766222731e776b874668004b (build)

@@ -26,7 +26,6 @@ export const useBadge_unstable = (props: BadgeProps, ref: React.Ref<HTMLElement>
},
root: getNativeElementProps('div', {
ref,
'aria-hidden': true,
Copy link
Contributor

Choose a reason for hiding this comment

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

does this also change accessibility / DOM of avatar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does. I believe @smhigley wanted that hidden removed from all badges. I'll confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, I don't think Badge should've been hidden, since it often contains meaningful text or graphics. And when it doesn't, it shouldn't be exposed even without aria-hidden :).

I think exposing its content is a much safer default than hiding it.

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case the best practices need to be updated:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

change moved to #22858

@@ -22,9 +22,11 @@ type PresenceBadgeCommons = {
* @default false
*/
outOfOffice: boolean;

strings: Record<PresenceBadgeStatus, string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

one thing to consider - we would require to provide all strings for each badge. Given there might be multiple badges on the page, each should get all texts and for each only one text will be used at a time, this requires consumers to either gather texts multiple times or create some advanced logic to reduce perf impact. Have you considered other alternatives that would require getting only texts that are being rendered?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jurokapsiar do you think the type should instead be Partial<Record<PresenceBadgeStatus, string>>? That would enable authors to only add a subset of strings, but at the detriment of providing warnings when strings are accidentally missed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I added partial into the prop, I wasn't aware that the original approach would require all of the strings to be provided. With partial a user could override a single value

Copy link
Contributor

Choose a reason for hiding this comment

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

we can just have one prop for the current string - the user knows what status they are setting, they can pair it with appropriate string.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

AFAIR we didn't make a decision about localisation approach (#19258). It would be great to define that the approach before an actual implementation.

andrefcdias and others added 8 commits May 3, 2022 13:02
* RFC: Update High contrast mode guidance

Updates guidance for high contrast support in Fluent UI. Emphasizes that
the library supports windows high contrast mode by default and explains
the context behind the hard coded high contrast theme

* update
…components subfolder (microsoft#22756)

* move react-spinbutton

* move react-spinner

* move react-tooltip

* Change Files
…icrosoft#22708)

* style: fix formatting issues introduced by manually renaming package

* feat(v9): migrate card,dialog,image to ship rolluped only dts

* generate change files

* fixup! feat(v9): migrate card,dialog,image to ship rolluped only dts

* fixup! generate change files

* fix(scripts): make sure api-extractor always runs for v9 no matter if --min flag was used
- Improve consistency between v8 and v9 examples
- Add more v8->v9 prop mappings for ChoiceGroupOption -> Radio
Prior to this change when a Radio was marked as "required" a red
asterisk was shown on the Radio's label to visually indicate it was
required.

This change no longer passes the `required` prop to the Label
subcomponent so this visual indicator is not shown. The Radio input is
still marked with the `required` attribute so it continues to be
semantically required.

As Radio is intended to be used inside a RadioGroup which should have a
label for the entire group the Label for the group should disply the
required indicator.

The required indicator can still be shown on the Label by passing the
`required` prop to the `label` slot via shorthand.
…22768)

* Updated to use correct pseudo-elements

* yarn change
bsunderhus and others added 8 commits May 5, 2022 08:18
…soft#22457)

* chore(react-theme): Generate theme tokens using token pipeline

* update type

* Address PR comments

* Do not register the script as just task

* Update paths to reflect the new location of react-theme

* Change file
* doc(react-theme): Add TypographyStyles story

* Changelog

* Comment formatting functions
…utton,conf-griffel,context-selector to ship rolluped only dts (microsoft#22823)

* feat(v9): migrate keyboard-keys,priority-overflow,alert,aria,avatar,button,conf-griffel,context-selector to ship rolluped only dts

* generate change files
* check mouse down

* e2e fix

* revert dialog change
* fix: pill behavior

* fix: Pill by adding default accessibility and considering selectable to add onclick

* chore: remove unstable comment

* chore: add missing role prop

* chore: handle dismiss and actionable

* chore: add role button when selectable

* chore: add changelog
@micahgodbolt
Copy link
Member Author

@layershifter the RFC is still in PR, but there doesn't seem to be any objections to the notion that controls have a 'strings' prop. https://github.com/microsoft/fluentui/pull/19258/files#diff-402680c7656a943bb0bfe680388a337152d239b5f063aeade9c1f7b1d903cf45R153

We might not have 100% agreement on how global/contextual strings are going to work, but regardless of our decisions, it shouldn't affect the need for having a strings prop.

We also already have several controls that have a strings prop, so it seems we've already come to the agreement that this prop is a proper first step to supporting i18n.

  const dropdownStrings = {
    ...locale.strings.global, // only if this component actually uses global strings
    ...locale.strings.Dropdown,
    ...props.strings,
  };

@micahgodbolt micahgodbolt mentioned this pull request May 5, 2022
2 tasks
@micahgodbolt
Copy link
Member Author

well crap...bad merge. going to close this PR down

@layershifter
Copy link
Member

@layershifter the RFC is still in PR, but there doesn't seem to be any objections to the notion that controls have a 'strings' prop. https://github.com/microsoft/fluentui/pull/19258/files#diff-402680c7656a943bb0bfe680388a337152d239b5f063aeade9c1f7b1d903cf45R153

We might not have 100% agreement on how global/contextual strings are going to work, but regardless of our decisions, it shouldn't affect the need for having a strings prop.

I commented there, I don't think that strings prop is future-proof solution and there are consequences with it. It's sad that it leaked to stable components without proper discussion around it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(v9) PresenceBadge: add labels based on status prop