-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[DatePicker] Add locale prop for Date Picker i18n #874
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
Conversation
👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack. |
<Day | ||
key={dayIndex} | ||
// eslint-disable-next-line react/jsx-no-bind | ||
onHover={onHover.bind(null, lastDayOfMonth)} |
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.
Not sure if we still need these .bind()
's since we're using @autobind
below..
..didn't want to change too much unrelated in this PR though
3893605
to
de691a2
Compare
"format": "sewing-kit format", | ||
"ts": "tsc --noEmit", | ||
"test": "sewing-kit test", | ||
"test": "env NODE_ICU_DATA=$(node-full-icu-path) sewing-kit test", |
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.
This new dependency is to enable Intl to be tested with different locales in jest, where prior it would just default to english no matter what locale you specify in the test..
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.
Code and test coverage look 🥇 but there's a couple problems with the 🎩
- Now that the weekday text ranges from 1-4 characters, the horizontal alignment is off in certain languages because currently padding is used for spacing, which doesn't collapse. Changing the
Weekday
class to use margin instead helped. I landed on changingpadding: spacing(tight);
tomargin: spacing(tight) auto;
and 🔥the nested wildcard selector with the negative margin rule.
padding: spacing(tight); |
margin: spacing(tight) auto; |
---|---|
![]() |
![]() |
- There's a visible flash/re-render when the date picker component is placed in a popover (on initial render only). I saw it first in the automatic discount detail view, but it's reproducible in the playground (code below) Since this is the typical use case, I don't think it'll be safe to ship this until we figure what's going on there.
- I thought maybe it was just because of some old code that needed to be refactored in
Month
, but even after changing Month to a class it still flashes on first re-render 🤔
- I thought maybe it was just because of some old code that needed to be refactored in
Click to view collapsed playground code
import * as React from 'react';
import {Page, DatePicker, Popover, Button} from '@shopify/polaris';
interface State {
active: boolean;
month: number;
year: number;
selected: {
start: Date;
end: Date;
};
}
export default class Playground extends React.Component<never, State> {
state = {
active: false,
month: 1,
year: 2018,
selected: {
start: new Date('Wed Feb 07 2018 00:00:00 GMT-0500 (EST)'),
end: new Date('Wed Feb 07 2018 00:00:00 GMT-0500 (EST)'),
},
};
render() {
const {month, year, selected} = this.state;
const activator = (
<Button onClick={this.togglePopover}>More actions</Button>
);
return (
<Page title="Playground">
<Popover
active={this.state.active}
activator={activator}
onClose={this.togglePopover}
>
<Popover.Section>
<DatePicker
locale="en"
month={month}
year={year}
onChange={this.handleChange}
onMonthChange={this.handleMonthChange}
selected={selected}
/>
</Popover.Section>
</Popover>
</Page>
);
}
handleChange = (value) => {
this.setState({selected: value});
};
handleMonthChange = (month, year) => {
this.setState({
month,
year,
});
};
togglePopover = () => {
this.setState(({active}) => {
return {active: !active};
});
};
}
Hi @joshuajay! Let us know if we can help answer any questions about feedback. |
4b091c1
to
b472c1a
Compare
Thanks @chloerice and @dpersing ! I've included the suggested CSS changes, and also figured out what was causing the re-render. Since I changed the the way the visible month / year props are passed down to be a Date object instead, and it was re-creating a new Date object each time, the props check to determine if it should re-render was counting each date object as different, even if they were the same date.
Update: scratch that last attempt, it ended up breaking more than it fixed, so instead I've opted to change the Month props back to what they were, which actually simplifies a bunch of things so it's definitely better this way 👍 |
b472c1a
to
99ef5f2
Compare
Cool, will take another 👀 |
@chloerice Have you had a chance to take another look? |
role="gridcell" | ||
> | ||
{date} | ||
{Intl.DateTimeFormat(locale, {day: 'numeric'}).format(day)} |
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.
disableDatesAfter, | ||
weekStartsOn = Weekdays.Sunday, | ||
polaris: {intl}, | ||
locale = 'en', |
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.
Can't we get the locale at the provider level or something like that? Or maybe not?
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 more for the Polaris team to answer, but is this a pattern they want to have in their library for their components?
@dpersing Sorry I forgot my recent convos with @joshuajay were in Slack! I took another look and the popover issue mentioned in my review is still there (double render flash on initial render). I thought maybe it was a browser cache issue and quit Chrome + rebooted and I still have it. |
WHY are these changes introduced?
Resolves #456
Currently the
DatePicker
component is only available in English but we should be able to display it in any valid language / locale.WHAT is this pull request doing?
Anywhere a specific date or range of dates is rendered visible (or screen readable) to the user, it is formatted in the specified locale as per the
Intl.DateTimeFormat
standard (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat).This includes the dates, months, years, and weekdays.
Something to note: The weekday labels will be slightly different in English after this change, instead of the 2-letter abbreviations that we have currently, Intl defines 'short' weekday names in the 'en' locale to be 3-letters.
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Tophatting is as simple as adding a valid locale to the component ('ja', 'it', 'pt-BR', 'de', 'zh').
Check that:
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist