feat(i18n): add i18n support #951
Conversation
gergelyke
force-pushed
the
feat/i18n
branch
2 times, most recently
from
2e3c075
to
6fc3237
Compare
src/datepicker/datepicker.js
Outdated
| if (__DEV__ && this.props['aria-label']) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `Providing i18n data through properties will be removed in the next major version. Please use the LocalProvider.`, |
There was a problem hiding this comment.
Do we really want to remove the props? They are straightforward to use and can be handy for some tweaks and overrides even when the LocalProvider is used. Also, it's not a big overhead for us and seems like an unnecessary breaking change that requires a lot of grunt work to fix.
// The only thing I would change is to group them into a single this.props.translate object so it's consistent across all components and doesn't pollute the top-level props but that ship already sailed.
There was a problem hiding this comment.
That can work too - the idea behind it was to make sure we only have a single API interface to maintain (and make it straightforward from a consumer point of view too, which way they have to if they wany i18n), but I guess we can go either way.
What do you think @nadiia @tajo @schnerd @chasestarr ?
There was a problem hiding this comment.
Let's keep the props as they are - I am going to update this PR
There was a problem hiding this comment.
Agree on keeping the aria attr in props 👍 For those who don't use i18n would be slightly too much work to do in order to change those if removed, specifically for an input case, for example.
| }; | ||
|
|
||
| export type LocaleT = {| | ||
| datepicker: DatepickerLocaleT, |
There was a problem hiding this comment.
Should we be overriding the English locale and keeping all LocaleT props optional?
There was a problem hiding this comment.
I mean user can do that but it should be a deepMerge and seems nice to do it by default.
There was a problem hiding this comment.
that makes perfect sense 👍
| @@ -170,7 +162,7 @@ export default class Datepicker extends React.Component< | |||
| <InputComponent | |||
| aria-disabled={this.props.disabled} | |||
| aria-label={ | |||
| locale.datepicker.ariaLabel || this.props['aria-label'] | |||
| this.props['aria-label'] || locale.datepicker.ariaLabel | |||
There was a problem hiding this comment.
Nice. I wanted to point this one out. 👍
| const {overrides = {}} = props; | ||
| const [Root, rootProps] = getOverrides(overrides.Root, StyledRoot); | ||
|
|
||
| return ( | ||
| <Root aria-label={props.ariaLabel} {...rootProps}> | ||
| <Root |
There was a problem hiding this comment.
could context consumer be placed here rather than in the wrapper component? not sure why locale needs to be provided through props
There was a problem hiding this comment.
yeah, sadly enyzme does not fully support contexts, so sadly the test framework affected our production code :(
is this correct @tajo ?
There was a problem hiding this comment.
Yea, this is a workaround for Enzyme. It can't shallow render the renderProp.
There was a problem hiding this comment.
could we simply mount and avoid the indirection? i've found that code bases grow in puzzling ways when ad hoc refactoring to enable test patterns
There was a problem hiding this comment.
I agree, 100%.
We've tried that too, it won't work - I think Vojtech will want to bring up the issues with enzyme, and start a chat on what we can do about it
for now, would you be ok with it as is, and fix it once we figure out what to do with enzyme?
There was a problem hiding this comment.
for sure - i would personally prefer to remove the test than add the additional indirection, but up to you
| {...sharedProps} | ||
| {...getOverrideProps(CloseOverride)} | ||
| {...getOverrideProps(DialogOverride)} |
Co-Authored-By: gergelyke <gergelyke@users.noreply.github.com>
this is a PoC implementation for the i18n support - please let me know what you think, and if everyone is onboard, I'll extend it for the rest of the codebase in this PR