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

[docs] Update mui-x on material-ui navigation #31810

Merged
merged 21 commits into from
Mar 24, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Mar 15, 2022

Changes

  1. Added the redirects for those who directly access the old URL (will land on the MUI X docs)

  2. Update the navigation UI by grouping data-grid & date-picker under MUI X and mark the lab as legacy.

    Before After
    Screen Shot 2565-03-15 at 10 37 50 Screen Shot 2565-03-22 at 10 55 32
  3. Added a deprecated page as a summary of the afford and provide useful links to other resources.
    Preview: https://deploy-preview-31810--material-ui.netlify.app/material-ui/lab-date-and-time-pickers/

@siriwatknp siriwatknp added the docs Improvements or additions to the documentation label Mar 15, 2022
@mui-bot
Copy link

mui-bot commented Mar 15, 2022

No bundle size changes

Generated by 🚫 dangerJS against b213f8b

@danilo-leal

This comment was marked as off-topic.

@flaviendelangle
Copy link
Member

Are we talking about the old doc infra or the new one

If it's the new one, do you want to list all the pages of the Data Grid and Date Pickers or just the 2-3 most important ?

@danilo-leal
Copy link
Contributor

This is the new infra (X docs separated from Core).

@siriwatknp
Copy link
Member Author

This is the new infra (X docs separated from Core).

If you mean the navigation in https://mui.com/x/react-data-grid/, it's beyond the scope of this PR and should be discussed with the X team.

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

If you mean the navigation in https://mui.com/x/react-data-grid/, it's beyond the scope of this PR and should be discussed with the X team.

Ah, then ignore my comment above! I have just now realized that this is the MUI X reference within Material UI docs. In that sense, I think it's all good! I've just reviewed a bit of the wording but I'll be requesting Sam's review for further improvement.

@siriwatknp
Copy link
Member Author

I've just reviewed a bit of the wording but I'll be requesting Sam's review for further improvement.

Thanks, that's a lot better! I'll commit and wait for another review from Sam.

Co-authored-by: danilo leal <67129314+danilo-leal@users.noreply.github.com>
siriwatknp and others added 4 commits March 16, 2022 17:25
Co-authored-by: danilo leal <67129314+danilo-leal@users.noreply.github.com>
@@ -0,0 +1,19 @@
# Date / Time pickers
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for capitalizing Time here? The headline seems to be referring to the general category of these components, as opposed to <DatePicker> and <TimePicker> themselves.

Also, I think it looks a lot cleaner to ditch the "/" in favor of "and."

Suggested change
# Date / Time pickers
# Date and time pickers

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suppose it's because you can also have <DateTimePicker>. But I personally think it's way cleaner the way you're proposing here.

@samuelsycamore
Copy link
Contributor

This page talks about date and time pickers, but the blog post about the move seems to suggest that it is just the date pickers moving. Or does the x-date-picker package include time pickers?

danilo-leal and others added 7 commits March 16, 2022 20:54
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
@siriwatknp
Copy link
Member Author

This page talks about date and time pickers, but the blog post about the move seems to suggest that it is just the date pickers moving. Or does the x-date-picker package include time pickers?

Yes, we meant the same thing (the whole pickers family). Any suggestion to make it clear?

@flaviendelangle
Copy link
Member

@samuelsycamore we have a lot of pickers (date picker, time picker, date time picker, date range picker, year picker, ...)

We chose to name the package @mui/x-date-pickers (with an s) instead of just @mui/x-pickers for instance because you could have pickers that are not related and that would go in another package (an emoji picker for instance).

And on the other hand, being more precise, like @mui/x-date-and-time-pickers makes the name very long and still is not exhaustive.

@danilo-leal
Copy link
Contributor

danilo-leal commented Mar 17, 2022

you could have pickers that are not related and that would go in another package

Maybe I'm might be coming late to this discussion (and this PR seems like not the best place to do it) but... wouldn't it make more sense to use x-pickers as a broader package name that includes each and every picker we come to develop? Having the package be x-date-pickers does spark a bit of confusion at first given it creates an expectation to have only date pickers. And, at some point, I'll also figure out that there are date time pickers and even pickers in general there as well.

Leaving it broader also gives the benefit of the doubt for the future — what if the emoji picker ends up sounding like a good idea one or two years from now? We'd just include it in the x-pickers package and there you go. No need to create x-emoji-pickers. It's just an example but I think there is a benefit.

@flaviendelangle
Copy link
Member

flaviendelangle commented Mar 17, 2022

@danilo-leal here is the Slack discussion

I don't think the term "picker" creates a component family consistent enough to group them in a single package.

For me this package is only intended for pickers that have something to do with to time.
An emoji picker or a color picker should not be in this package.

@danilo-leal
Copy link
Contributor

Shoot, seeing now that I even voted in favor of x-date-pickers 😅 But yeah, okay, don't think we should change the decision here or anything. But I'm afraid that x-date-pickers doesn't put clearly across that you'll find a time picker there as well (time in terms of interacting with a clock not with date). It's reasonable to expect anything date-related but time may come as a surprise (not a bad one but just something not right in your face).

@samuelsycamore
Copy link
Contributor

In this case, I would propose that we refer to x-date-pickers collectively as "date and time pickers," which can be shortened to just "pickers" or "picker components" after the first usage in a document.

@siriwatknp
Copy link
Member Author

@samuelsycamore @danilo-leal I updated the url to be /material-ui/lab-date-and-time-pickers/ (from what I understand in the comments). Is there anything that needs to be done in this PR?

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

I think we're good to go with this one!

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

Just a teeny tiny tweak to the title, otherwise it looks good to me!

…s.md

Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 23, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 23, 2022
@siriwatknp siriwatknp merged commit 567a131 into mui:master Mar 24, 2022
@siriwatknp siriwatknp mentioned this pull request Mar 29, 2022
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants