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.

Sign up for GitHub

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

Jump to bottom

[fields] Implement empty visual state #8069

Merged
merged 8 commits into from
Mar 3, 2023
Merged

[fields] Implement empty visual state #8069

merged 8 commits into from
Mar 3, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Feb 27, 2023 •
edited by oliviertassinari
Loading

Fix #4592
Doc preview: https://deploy-preview-8069--material-ui-x.netlify.app/x/react-date-pickers/fields/

Change empty fields value behavior:

  • use placeholder/format as placeholder instead of value when no value is present and field is not focused
  • use value when field is focused or value exists

Before

Screenshot 2023-03-05 at 00 31 38

After

Screenshot 2023-03-05 at 00 30 59

Screenshot 2023-03-05 at 00 36 06

@LukasTy LukasTy added component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature feature: Keyboard editing Related to the pickers keyboard edition labels Feb 27, 2023
@LukasTy LukasTy self-assigned this Feb 27, 2023
@LukasTy LukasTy changed the title [fields] Implement empty input state [fields] Implement empty visual state Feb 27, 2023
@mui-bot
Copy link

mui-bot commented Feb 27, 2023 •
edited
Loading

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8069--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 730.3 1,081.9 794.2 882.76 133.89
Sort 100k rows ms 658.9 1,421.2 1,421.2 1,035.28 253.155
Select 100k rows ms 303 482.1 310.9 342.92 69.676
Deselect 100k rows ms 154.9 367.6 221.7 255.56 83.674

Generated by 🚫 dangerJS against 38fc7b4

@LukasTy LukasTy marked this pull request as ready for review February 28, 2023 13:38
@LukasTy
Copy link
Member Author

LukasTy commented Feb 28, 2023

@joserodolfofreitas You might also be interested to take a look at this change. 🤔
Maybe you expected something different or have some UX concerns?

LukasTy
LukasTy commented Feb 28, 2023
View reviewed changes
packages/x-date-pickers/src/DateField/tests/selection.DateField.test.tsx
expectInputValue(input, 'MM / DD / YYYY');

input.setSelectionRange(9, 11);
Copy link
Member Author

Choose a reason for hiding this comment

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

Was this a mistake previously, that somehow did not fail the test? 🤔 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I think @oliviertassinari wrote the initial version of this test
We try to mimic a touch focus interaction, but I did not check what was the correct execution order of the events.

Copy link
Member

@oliviertassinari oliviertassinari Mar 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, this test comes from #6207.

It seems that this test caught a regression, changing the test hides the bug. See how on https://deploy-preview-8069--material-ui-x.netlify.app/x/react-date-pickers/fields/#fields-to-edit-a-single-element, now I need to tap twice to select the year:

20230305-002805-333.mp4

One tap is enough on https://next.mui.com/x/react-date-pickers/fields/#fields-to-edit-a-single-element.

Copy link
Member

Choose a reason for hiding this comment

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

This behavior change is intended. It was a trade off between having a placeholder and being able to pick a section in a non focused input in a single click.

Copy link
Member

@oliviertassinari oliviertassinari Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, OK, so using the <input> placeholder attribute over applying the placeholder color. This sounds simpler for developers when they provide a custom textbox.

@flaviendelangle
Copy link
Member

I tested it quickly, it looks great ! 🥳

Do you think we could avoid the "focus all" flickering ?

Screencast.2023-02-28.16.28.41.mp4

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2023
@LukasTy
Copy link
Member Author

LukasTy commented Mar 1, 2023

Do you think we could avoid the "focus all" flickering ?

Thanks for bringing it up! 👍
Seems that we only needed this extra check. 🤔

@flaviendelangle
Copy link
Member

Seems that we only needed this extract check

Seems to do the trick indeed 👍

joserodolfofreitas
joserodolfofreitas approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Feels much better now! Thanks for tackling this before the stable release!

@LukasTy LukasTy merged commit d7fedbb into mui:next Mar 3, 2023
@LukasTy LukasTy deleted the fields-empty-placeholder branch March 3, 2023 08:05
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 4, 2023 •
edited
Loading

Great design improvement! I have updated the PR's description so developers can more easily understand what this is about.

@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaviendelangle flaviendelangle flaviendelangle approved these changes

@oliviertassinari oliviertassinari oliviertassinari left review comments

@joserodolfofreitas joserodolfofreitas joserodolfofreitas approved these changes

@alexfauquette alexfauquette Awaiting requested review from alexfauquette

Assignees

@LukasTy LukasTy

Labels
component: pickers This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer enhancement This is not a bug, nor a new feature feature: Keyboard editing Related to the pickers keyboard edition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[pickers] Missing emptyLabel feature on datepicker and timepicker
5 participants
@LukasTy @mui-bot @flaviendelangle @oliviertassinari @joserodolfofreitas