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] Add Base installation page #30969

Merged
merged 13 commits into from
Feb 24, 2022
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 8, 2022

Added installation page for Base as a starting point for naming discussion 😂

What should be the name of the product?

  • The conclusion is to go with "MUI Base" for now.
  • The version identifier is shortened (remove .0.0), otherwise it causes double lines.

Screen Shot 2565-02-23 at 17 52 54

@michaldudak feel free to add more content here or in separate PRs (your call). This PR is intended to be a template for other pages.

Note: please neglect ProductDrawer refinement from this PR

Preview: https://deploy-preview-30969--material-ui.netlify.app/base/getting-started/installation/


@siriwatknp siriwatknp requested a review from a team February 8, 2022 07:12
@siriwatknp siriwatknp marked this pull request as draft February 8, 2022 07:12
@mui-bot
Copy link

mui-bot commented Feb 8, 2022

No bundle size changes

Generated by 🚫 dangerJS against 72937bb

@@ -92,7 +92,7 @@ function AppLayoutDocs(props) {
productName = 'Material UI';
}
if (asPathWithoutLang.startsWith('/base')) {
productName = 'Base UI';
productName = 'MUI Base';
Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on #30960

Copy link
Member

@oliviertassinari oliviertassinari Feb 8, 2022

Choose a reason for hiding this comment

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

I don't think that 1. is an option as it's not self-sustained: need to be able to search it on Google and find what you are looking for.

For 2 vs. 3. No preferences, both could fly.

Maybe the only note I would have is either /base in the URL is enough or either it should match the name. I think that depending on the option, here is what a great URL would look like:

  1. => https://mui.com/base-ui/
  2. => https://mui.com/base/

Copy link
Member

@hbjORbj hbjORbj Feb 8, 2022

Choose a reason for hiding this comment

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

With "MUI Core" and "MUI X" above and below, I think "MUI Base" would match better with consistency.

p.s. I think renaming System to "MUI System" and "Data Grid" to "MUI Data Grid" would make the names look more in order as well.

Copy link
Member

@michaldudak michaldudak Feb 9, 2022

Choose a reason for hiding this comment

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

Agree with @oliviertassinari - option 1 isn't the best. I don't have a strong preference. Also, since we decided not to follow the "MUI *" pattern for all the products, it opens up the possibility to rename Base to something more distinct.

Let's keep MUI Base for now so we don't keep the PR open for too long. It's easy to change it later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with Michal, to not use too much time here to discuss the name and instead maybe focus on the installation walkthrough itself. We can better resolve naming by looking at everything at once holistically later.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Feb 8, 2022
@siriwatknp siriwatknp marked this pull request as ready for review February 22, 2022 10:22
@siriwatknp
Copy link
Member Author

@michaldudak I have changed to Base UI instead. I think it is ready for review.

@oliviertassinari oliviertassinari mentioned this pull request Feb 22, 2022
32 tasks
@mnajdova
Copy link
Member

mnajdova commented Feb 22, 2022

@michaldudak I have changed to Base UI instead. I think it is ready for review.

I would use MUI Base everywhere and merge. Until we are certain about the name, why using a new one?

@michaldudak
Copy link
Member

Agree - let's use MUI Base for now.

@@ -480,13 +462,17 @@ function AppNavDrawer(props) {
onClick={(event) => {
setAnchorEl(event.currentTarget);
}}
endIcon={<ArrowDropDownRoundedIcon fontSize="small" sx={{ ml: -0.5 }} />}
endIcon={
Copy link
Member

@oliviertassinari oliviertassinari Feb 22, 2022

Choose a reason for hiding this comment

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

Off-topic.

I have noticed this behavior for Windows users, I'm not sure how to solve this layout constraint issue:

layout.shift.mp4

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 can stretch the space a little bit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Screen Shot 2565-02-23 at 09 38 18

I have to remove.0.0, otherwise I can't think of a way to reduce the space.

@siriwatknp
Copy link
Member Author

@michaldudak I have changed to Base UI instead. I think it is ready for review.

I would use MUI Base everywhere and merge. Until we are certain about the name, why using a new one?

I might have misunderstood the previous comments, will change to MUI Base

@siriwatknp
Copy link
Member Author

@michaldudak It is changed to MUI Base, do you see anything that might be missing?

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Just two comments from my side

sx={(theme) => ({
py: 0.1,
minWidth: 0,
fontSize: theme.typography.pxToRem(13),
fontWeight: 500,
lineHeight: 0,
// lineHeight: 0,
Copy link
Member

Choose a reason for hiding this comment

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

Is this to be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, thanks!.

metadata="MUI Core"
versionSelector={renderVersionSelector([
{ text: `v${basePkgJson.version}`, current: true },
{ text: `v${basePkgJson.version.replace('.0.0', '')}`, current: true },
Copy link
Member

@michaldudak michaldudak Feb 23, 2022

Choose a reason for hiding this comment

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

How will it work with versions like *.0.1?

Copy link
Member

Choose a reason for hiding this comment

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

@siriwatknp should we maybe extend a bit the width of the drawer instead of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak with 5.0.1, you won't have alpha-XX which works fine in one line.

@mnajdova need to increase the width for 12px to make it in one line.

@danilo-leal what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

About the width increase, sure!
Are we removing the alpha word then?

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, I don't think so. It should show the version from the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, great! It seemed like we would, asked for clarification!

@siriwatknp siriwatknp merged commit d588f5b into mui:master Feb 24, 2022
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.

7 participants