-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
RFC: conventions for internationalization/localization in converged components #19258
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 36a89f225774d37710b772eb0e0a467c0e53217e (build) |
📊 Bundle size report🤖 This report was generated against ce2d65af5c3fb0228a4a79e987809c6c765e2250 |
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 0033350:
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1000 | 984 | 5000 | |
BaseButton | mount | 1005 | 1010 | 5000 | |
Breadcrumb | mount | 2750 | 2767 | 1000 | |
ButtonNext | mount | 505 | 514 | 5000 | |
Checkbox | mount | 1671 | 1730 | 5000 | |
CheckboxBase | mount | 1485 | 1506 | 5000 | |
ChoiceGroup | mount | 5261 | 5204 | 5000 | |
ComboBox | mount | 1049 | 1051 | 1000 | |
CommandBar | mount | 10730 | 10799 | 1000 | |
ContextualMenu | mount | 6883 | 7015 | 1000 | |
DefaultButton | mount | 1261 | 1221 | 5000 | |
DetailsRow | mount | 4103 | 4068 | 5000 | |
DetailsRowFast | mount | 4064 | 4028 | 5000 | |
DetailsRowNoStyles | mount | 3789 | 3859 | 5000 | |
Dialog | mount | 2475 | 2532 | 1000 | |
DocumentCardTitle | mount | 170 | 165 | 1000 | |
Dropdown | mount | 3463 | 3529 | 5000 | |
FluentProviderNext | mount | 7365 | 7299 | 5000 | |
FluentProviderWithTheme | mount | 348 | 345 | 10 | |
FluentProviderWithTheme | virtual-rerender | 90 | 94 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 483 | 455 | 10 | |
FocusTrapZone | mount | 1920 | 1962 | 5000 | |
FocusZone | mount | 1908 | 1898 | 5000 | |
IconButton | mount | 1926 | 1943 | 5000 | |
Label | mount | 363 | 365 | 5000 | |
Layer | mount | 3303 | 3186 | 5000 | |
Link | mount | 517 | 529 | 5000 | |
MakeStyles | mount | 1953 | 1947 | 50000 | |
MenuButton | mount | 1618 | 1644 | 5000 | |
MessageBar | mount | 2164 | 2189 | 5000 | |
Nav | mount | 3488 | 3561 | 1000 | |
OverflowSet | mount | 1198 | 1208 | 5000 | |
Panel | mount | 2442 | 2444 | 1000 | |
Persona | mount | 903 | 906 | 1000 | |
Pivot | mount | 1552 | 1562 | 1000 | |
PrimaryButton | mount | 1393 | 1402 | 5000 | |
Rating | mount | 8531 | 8645 | 5000 | |
SearchBox | mount | 1460 | 1460 | 5000 | |
Shimmer | mount | 2915 | 2849 | 5000 | |
Slider | mount | 2112 | 2136 | 5000 | |
SpinButton | mount | 5453 | 5386 | 5000 | |
Spinner | mount | 445 | 436 | 5000 | |
SplitButton | mount | 3428 | 3472 | 5000 | |
Stack | mount | 577 | 559 | 5000 | |
StackWithIntrinsicChildren | mount | 1883 | 1874 | 5000 | |
StackWithTextChildren | mount | 5255 | 5235 | 5000 | |
SwatchColorPicker | mount | 11202 | 11103 | 5000 | |
Tabs | mount | 1525 | 1516 | 1000 | |
TagPicker | mount | 2885 | 2858 | 5000 | |
TeachingBubble | mount | 13797 | 13700 | 5000 | |
Text | mount | 453 | 489 | 5000 | |
TextField | mount | 1536 | 1512 | 5000 | |
ThemeProvider | mount | 1228 | 1280 | 5000 | |
ThemeProvider | virtual-rerender | 621 | 642 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1968 | 2042 | 5000 | |
Toggle | mount | 877 | 868 | 5000 | |
buttonNative | mount | 127 | 121 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
AlertMinimalPerf.default | 320 | 283 | 1.13:1 |
TextMinimalPerf.default | 425 | 392 | 1.08:1 |
LayoutMinimalPerf.default | 433 | 404 | 1.07:1 |
TreeWith60ListItems.default | 205 | 191 | 1.07:1 |
VideoMinimalPerf.default | 737 | 692 | 1.07:1 |
ChatDuplicateMessagesPerf.default | 337 | 319 | 1.06:1 |
DropdownManyItemsPerf.default | 804 | 762 | 1.06:1 |
ImageMinimalPerf.default | 440 | 417 | 1.06:1 |
ListMinimalPerf.default | 587 | 552 | 1.06:1 |
RadioGroupMinimalPerf.default | 514 | 486 | 1.06:1 |
MenuMinimalPerf.default | 959 | 914 | 1.05:1 |
ReactionMinimalPerf.default | 438 | 420 | 1.04:1 |
TableMinimalPerf.default | 457 | 441 | 1.04:1 |
ToolbarMinimalPerf.default | 1050 | 1014 | 1.04:1 |
AccordionMinimalPerf.default | 174 | 169 | 1.03:1 |
AnimationMinimalPerf.default | 450 | 437 | 1.03:1 |
AvatarMinimalPerf.default | 225 | 219 | 1.03:1 |
ButtonSlotsPerf.default | 631 | 613 | 1.03:1 |
CardMinimalPerf.default | 650 | 631 | 1.03:1 |
ChatMinimalPerf.default | 746 | 723 | 1.03:1 |
FormMinimalPerf.default | 475 | 460 | 1.03:1 |
HeaderMinimalPerf.default | 419 | 407 | 1.03:1 |
RefMinimalPerf.default | 261 | 253 | 1.03:1 |
SkeletonMinimalPerf.default | 395 | 385 | 1.03:1 |
TableManyItemsPerf.default | 2176 | 2108 | 1.03:1 |
TooltipMinimalPerf.default | 1163 | 1132 | 1.03:1 |
TreeMinimalPerf.default | 885 | 859 | 1.03:1 |
AttachmentMinimalPerf.default | 178 | 175 | 1.02:1 |
CarouselMinimalPerf.default | 518 | 509 | 1.02:1 |
ChatWithPopoverPerf.default | 424 | 414 | 1.02:1 |
DialogMinimalPerf.default | 828 | 808 | 1.02:1 |
DividerMinimalPerf.default | 412 | 405 | 1.02:1 |
GridMinimalPerf.default | 381 | 372 | 1.02:1 |
HeaderSlotsPerf.default | 861 | 842 | 1.02:1 |
InputMinimalPerf.default | 1386 | 1354 | 1.02:1 |
ListWith60ListItems.default | 729 | 713 | 1.02:1 |
PopupMinimalPerf.default | 645 | 633 | 1.02:1 |
PortalMinimalPerf.default | 196 | 193 | 1.02:1 |
SegmentMinimalPerf.default | 402 | 394 | 1.02:1 |
SplitButtonMinimalPerf.default | 4645 | 4542 | 1.02:1 |
IconMinimalPerf.default | 691 | 680 | 1.02:1 |
CheckboxMinimalPerf.default | 2933 | 2902 | 1.01:1 |
DatepickerMinimalPerf.default | 5882 | 5814 | 1.01:1 |
EmbedMinimalPerf.default | 4574 | 4530 | 1.01:1 |
ItemLayoutMinimalPerf.default | 1364 | 1355 | 1.01:1 |
ListNestedPerf.default | 610 | 605 | 1.01:1 |
MenuButtonMinimalPerf.default | 1832 | 1807 | 1.01:1 |
ProviderMinimalPerf.default | 1157 | 1141 | 1.01:1 |
AttachmentSlotsPerf.default | 1171 | 1173 | 1:1 |
ButtonMinimalPerf.default | 194 | 194 | 1:1 |
DropdownMinimalPerf.default | 3336 | 3320 | 1:1 |
LoaderMinimalPerf.default | 737 | 738 | 1:1 |
ProviderMergeThemesPerf.default | 1796 | 1804 | 1:1 |
CustomToolbarPrototype.default | 4236 | 4245 | 1:1 |
BoxMinimalPerf.default | 397 | 399 | 0.99:1 |
FlexMinimalPerf.default | 310 | 312 | 0.99:1 |
SliderMinimalPerf.default | 1759 | 1781 | 0.99:1 |
StatusMinimalPerf.default | 752 | 760 | 0.99:1 |
ButtonOverridesMissPerf.default | 1888 | 1918 | 0.98:1 |
ListCommonPerf.default | 706 | 722 | 0.98:1 |
TextAreaMinimalPerf.default | 574 | 585 | 0.98:1 |
LabelMinimalPerf.default | 414 | 427 | 0.97:1 |
RosterPerf.default | 1257 | 1330 | 0.95:1 |
|
||
To do this, I think it makes sense to add locale data to Fluent's `ProviderContext`, since it already includes `dir`. | ||
|
||
One straightforward option for how to do this would be to provide a locale object with static key/value pairs for each string. We could then import defaults for all strings where defaults make sense, which would allow us to also share global strings for things like 'close'/'remove'/'select all'/etc. So something like this (specific file names and values are just for show): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have a single file for all strings:
- Bad for bundle size. The extreme example is once we converge Calendar/DatePicker, that has a LOT of strings, but relatively few pages will need that component (especially on initial page load).
- It's against the patterns we've used so far to have one low-level package which also knows about individual components.
But standardizing a location/name for default strings within each package would make sense. I also agree it might make sense to have very common strings defined once in a common location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree there with @ecraig12345. We have an example of such objects in Northstar:
GZIP sizes of individual exports
- Themes in that case are not tree-shakable i.e. include styles even for unused components
- Without a valid theme components will not have styles i.e. increases initial page load
For a locale object it will be the same thing as every additional string that will be added - will increase bundle size for every component 😥
For example, react-spectrum
handles such strings per component: https://github.com/adobe/react-spectrum/tree/main/packages/%40react-spectrum/pagination.
In the same, we should keep config simple for consumers. FluentProvider
can host a name of locale and use similar locale detection as in: https://github.com/adobe/react-spectrum/blob/1e2b7f280a04ead83e21b27fa580eb189e5f4186/packages/%40react-aria/i18n/src/useDefaultLocale.ts#L47-L50
BTW, SSR scenario is handled there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, Calendar/Datepicker's a great example of something we wouldn't want to ship all the time. Aside from date-time, I actually think we won't have many instances of default values for strings that are component-specific.
Assuming that's the case, how would you feel about one single file for global defaults (e.g. "close", "open", "filter", "{0} results", etc.)? We could start with that, and see if we even need to have component-specific default strings. If we do, we could start by importing those defaults directly from an in-package string default file in addition to the global default file. Since that would be an internal implementation detail, I don't think we'd need to worry too much about getting it perfect to start. As long as an author can always pass in strings through a strings
prop, how we organize our defaults shouldn't be a breaking change.
Does that sound like a good approach to you, and would that approach cover your bundle size and pattern concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from date-time, I actually think we won't have many instances of default values for strings that are component-specific.
ColorPicker 😆 but that's not scheduled any time soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that's the case, how would you feel about one single file for global defaults (e.g. "close", "open", "filter", "{0} results", etc.)?
If you plan to add these defaults to FluentProvider
, this will still increase bundle size for component like Button
or Text
that don't need these defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bit of a tradeoff since it would slightly increase the bundle size for someone using only the Button or Text components, but would decrease the size for someone using multiple components like Dialog, Combobox, SplitButton, List, etc. I'm also guessing the entire global default strings file won't be more than ~20 lines at a generous estimate, based on the Material and Ant Design precedents, so not a huge bundle size impact either way :).
The other benefit of having a set of global defaults is that it will ensure that there is only one string name for something like "close". That way a team translating our components won't need to worry about mapping the same translated string to both strings.close
and strings.closeButtonLabel
, just because we didn't manually check that our keys are consistent across all components.
For clarity, this proposal is strictly not about things like |
Per feedback, let's get an exhaustive list of strings that we're talking about across the entire library. If it is very few strings, it likely will not warrant a component and new package in order to define them. Perhaps we just standardize each component API instead. |
I'll leave the list of common strings to @smhigley but in case it's helpful, here are the special cases that have a lot of unique strings: fluentui/packages/date-time-utilities/src/dateFormatting/dateFormatting.defaults.ts Lines 38 to 83 in dab45a2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great and necessary proposal. I'm unaware of how big the bundle size impact will be but it's definitely something we need. Good stuff!
I went through the strings currently in Fluent v8 and Northstar, and these are the globals I think we'd want to include, based on those: Fluent global strings:
There were a few other strings that could potentially be globals, but are currently only used in one component:
|
Updated the RFC with specific global strings, and also a beta-specific minimal recommendation |
react-shared-contexts/src/ProviderContext/defaultStrings.ts | ||
|
||
```js | ||
export const defaultLocale { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how feasible it would be to maintain a global locale string object with our current component architecture.
Each of our components are encapsulated in their own packages, it makes perfect sense to colocate the strings with the components that use them.
By creating a global string object, we need to hoist another context above all other components to share strings, which breaks down the multi-package architecture a bit more. Modifying or removing strings become a breaking API change for an unknown number of components
Tree shaking would also need to be considered since we probably would not want components to bundle strings that are not used
|
||
```js | ||
export const defaultLocale { | ||
locale: 'en-US', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is information about the local useful for the library ? apps that need to be internationalized will have this information in abundance
|
||
## Problem Statement: | ||
|
||
In v8, the custom strings authors must define to use components are all over the place, and have no standard pattern. E.g. Datepicker uses a `strings` prop and imports defaults from an in-package module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a requirement for v9 to ship with default english strings ? it will not remove the requirement for products to define all these strings again, since they will need to do the translation through their pipelines.
shipping with a default locale only makes sense to me if we were to ship with all required M365 locales, otherwise the work to define the default strings would be duplicated by Fluent and consumers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence about DatePicker is slightly inaccurate. In v7 it was required to specify strings (with an English defaults object available). In v8 English strings are provided by default, though in the examples we still demonstrate importing and setting the strings to emphasize that localization is required.
Overall in v8, for the string-heavy components (DatePicker, Calendar, ColorPicker) we provide English defaults, possibly to reduce initial friction/potential confusion for people using them. For other components we don't provide default strings. I don't think we have a hard requirement either way for v9.
import { useTranslation } from 'react-i18next'; | ||
|
||
const App = () => { | ||
const { t, i18n } = useTranslation('es-ES'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we would force users to use our localization keys for their applications or force them to build a mapping layer between our strings and theirs if we support i18n this way. Is this acceptable for our partners ?
|
||
// defined in a default strings file | ||
const presenceDefaultStrings: PresenceBadgeStrings = { | ||
statusBusy: 'busy', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if users don't want to use localized strings, how would they do this ?
Is it better to force users to explicitly declare or remove strings ?
|
||
## Concrete actions for vNext beta | ||
|
||
The only action needed for beta release should be to ensure all strings provided through component props (excluding children/child content) are defined in a single `strings` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings
is not the best name for localisation purposes: there are cases when it should be actually functions. Especially when it goes to plurals for different languages:
<Component
strings={{
getApples: count => {
if (count === 0) return "No apples";
return `${count} apple(s)`;
}
}}
/>
Is it good idea to have functions under strings? Or there will be two props?
<Component
strings={{ busy: 'Busy' }}
functions={{
getApples: count => {
if (count === 0) return "No apples";
return `${count} apple(s)`;
}
}}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a prop for localization can also play a bad game with consistency in a big application where components are not localized consistently and each application area has own strings that don't match or are missing proper translation.
The value of strings
prop is also questionable when it comes to different languages. Do we really expect to have this?
function App() {
const locale = useLocale();
return (
<List>
<ListItem strings={SOME_SHARED_STRINGS[locale]} />
<ListItem strings={SOME_SHARED_STRINGS[locale]} />
<ListItem strings={SOME_SHARED_STRINGS[locale]} />
<ListItem strings={SOME_SHARED_STRINGS[locale]} />
{/* 50 times more */}
</List>
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we don't have an agreement and complete design, but it have been already leaked to existing components (#22775) to unblock localization I propose at least prefix it with unstable_
to have some room for changes.
* react-spinbutton: a11y updates Minor styling and story updates based on accessibility testing. * react-spinbutton: add labels to spinner buttons Adds default, English, labels to the increment and decrement buttons of SpinButton. This provides a default description for assistive tech users. This commit implements a new prop, `strings` per RFC microsoft#19258. Default values are provided for English and the strings support a token, `{step}` that will be replaced with the value of the `step` prop. The `{step}` token may be omitted. All string keys are required but invididual labels may be overridden by passing the `aria-label` prop to a slot. * react-spinbutton: update API snapshot * react-spinbutton: remove commented out TODOs
|
||
### 2. Investigate supporting string templates or functions | ||
|
||
We already have some cases where a simple string might work, but a template string would be better: for example `"{count} more people"` vs. `"more people"` in `AvatarGroup`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please note that the string format that MS uses is a bit different - it uses double parenthesis for arguments {{count}} more people
. If we decide to use it, we should make it configurable, or ideally just consume a callback that does the interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this is a good case where plurals need to be used as in different languages the string can change depending on the count - different wording for 0, 1, 2, 5 or more. That would be another use case for just consuming a callback.
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
An internationalization/localization proposal that includes:
locale
object added toProviderContext
defaultStrings
file