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

[docs] Create Pickers masked field recipe #13515

Merged
merged 56 commits into from
Oct 28, 2024
Merged

[docs] Create Pickers masked field recipe #13515

merged 56 commits into from
Oct 28, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jun 17, 2024 •
edited
Loading

@flaviendelangle flaviendelangle self-assigned this Jun 17, 2024
@flaviendelangle flaviendelangle added docs Improvements or additions to the documentation component: pickers This is the name of the generic UI component, not the React module! labels Jun 17, 2024
@mui-bot
Copy link

mui-bot commented Jun 17, 2024 •
edited
Loading

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

Updated pages:

Generated by 🚫 dangerJS against 15b55cc

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 8, 2024

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 8, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2024
@github-actions GitHub Actions
Copy link

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 24, 2024
@LukasTy LukasTy mentioned this pull request Sep 3, 2024
Closed
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 4, 2024
@flaviendelangle flaviendelangle mentioned this pull request Sep 5, 2024
Merged
4 tasks
@github-actions GitHub Actions
Copy link

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 7, 2024
@flaviendelangle flaviendelangle marked this pull request as ready for review October 7, 2024 12:39
michelengelen
michelengelen reviewed Oct 25, 2024
View reviewed changes
Copy link
Member

@michelengelen michelengelen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

just a single comment on the cursor positioning. LGTM otherwise!

docs/data/date-pickers/custom-field/custom-behavior/MaskedMaterialTextField.tsx
import { useValidation, validateDate } from '@mui/x-date-pickers/validation';

const MASK_USER_INPUT_SYMBOL = '_';
const ACCEPT_REGEX = /[\d]/gi;
Copy link
Member

Choose a reason for hiding this comment

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

nit: not so sure about this ... i definitely isn't needed, but are you intending to only check for a single digit here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the regexp that we used in v5.
This code came from the lab and I never really touched it a lot to be honested 😆

https://github.com/mui/mui-x/blob/v5.x/packages/x-date-pickers/src/internals/hooks/useMaskedInput.tsx#L26

But we don't have only digits, we have letters, at least for the meridiem.

Copy link
Member

Choose a reason for hiding this comment

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

oh right ... we could potentially improve this then and use a different regex to exclude anything else: [\d]|(a|p)m

docs/data/date-pickers/custom-field/custom-behavior/MaskedMaterialTextField.tsx
nextMaskChar &&
nextMaskChar !== MASK_USER_INPUT_SYMBOL
) {
// when cursor at the end of mask part (e.g. month) prerender next symbol "21" -> "21/"
Copy link
Member

Choose a reason for hiding this comment

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

could we also move the cursor to behind the '/'?
Right now it looks like this: 21|/, but preferably it would do this instead: 21/|

Copy link
Member Author

@flaviendelangle flaviendelangle Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In v5 the cursor was before the /
I do agree that having it after would be better, but I did not find a solution to do it 😬
You can play with the original behavior here: https://v5.mui.com/x/react-date-pickers/date-picker/

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, i thought so ... its not blocking IMO

@flaviendelangle @michelengelen
Update docs/data/date-pickers/custom-field/custom-field.md
9951d10 
Co-authored-by: Michel Engelen <[email protected]>
Signed-off-by: Flavien DELANGLE <[email protected]>
michelengelen
michelengelen approved these changes Oct 25, 2024
View reviewed changes
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.

since this works without problems in its current state i won't block it with nitty stuff! :P

mapache-salvaje
mapache-salvaje approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@mapache-salvaje mapache-salvaje left a comment

Choose a reason for hiding this comment

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

tiny copyedits here, otherwise text looks good

@LukasTy LukasTy changed the title [docs] New recipe of a masked field [docs] Create Pickers masked field recipe Oct 28, 2024
LukasTy
LukasTy approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👍
This is a really great improvement for users desiring the "old" behavior back. 💯
Should we cherry-pick it to v7? 🤔

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Oct 28, 2024 •
edited
Loading

Should we cherry-pick it to v7?

I'd say no 👼
It's not a bug fix and it's a reason for people to migrate

@flaviendelangle flaviendelangle merged commit 58682ef into mui:master Oct 28, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the explore-masked-recipe branch October 28, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasTy LukasTy LukasTy approved these changes

@mapache-salvaje mapache-salvaje mapache-salvaje approved these changes

@michelengelen michelengelen michelengelen approved these changes

@arthurbalduini arthurbalduini Awaiting requested review from arthurbalduini

@joserodolfofreitas joserodolfofreitas Awaiting requested review from joserodolfofreitas

@noraleonte noraleonte Awaiting requested review from noraleonte

Assignees

@flaviendelangle flaviendelangle

Labels
component: pickers This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[docs] Create a recipe of a masked-based field
5 participants
@flaviendelangle @mui-bot @LukasTy @michelengelen @mapache-salvaje