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

[pickers] Avoid relying on locale in Luxon isWithinRange method #11936

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Feb 5, 2024

Fixes #11622

We used Luxon API for comparing equality, but it also includes some metadata in comparison (like locale the date was created it).
With this change, we are only going to compare the difference in time.

@LukasTy LukasTy added plan: Pro Impact at least one Pro user regression A bug, but worse component: pickers This is the name of the generic UI component, not the React module! component: DateRangePicker The React component. labels Feb 5, 2024
@LukasTy LukasTy self-assigned this Feb 5, 2024
@LukasTy LukasTy added the needs cherry-pick The PR should be cherry-picked to master after merge label Feb 5, 2024
@mui-bot
Copy link

mui-bot commented Feb 5, 2024

Deploy preview: https://deploy-preview-11936--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 3f28587

Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:


describe('isWithinRange functionality', () => {
const adapter = new AdapterLuxon({ locale: 'en-US' });
it('should be equal with values in different locale', () => {
Copy link
Member

Choose a reason for hiding this comment

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

If possible, it would be great to move the test inside describeGregorianAdapter (there is a describe for isWithinRange).
That way we make sure that all adapters are coherent on this behavior.

We have a adapterFr object that is using the french locale so we should be able to use it to create the two dates and then use adapter to run isWithingRange with an en-US locale.

If I'm missing something and there is a technical limitation preventing us from doing it, it's of course not a deal breaker 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for a great suggestion! 🙏
Moved the test and tests it's reliability. 😉

@@ -324,8 +324,8 @@ export class AdapterLuxon implements MuiPickersAdapter<DateTime, string> {

public isWithinRange = (value: DateTime, [start, end]: [DateTime, DateTime]) => {
return (
value.equals(start) ||
value.equals(end) ||
this.isEqual(value, start) ||
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

The fix makes a lot of sense, thanks for taking care ok it 👌

@LukasTy LukasTy merged commit af04654 into mui:next Feb 5, 2024
15 checks passed
@LukasTy LukasTy deleted the fix-luxon-is-within-range branch February 5, 2024 15:07
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: DateRangePicker The React component. component: pickers This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge plan: Pro Impact at least one Pro user regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DateRangePicker] Date range is not highlighted properly with AdapterLuxon
4 participants