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

[unstyled] Create package and move SliderUnstyled there #23270

Merged
merged 30 commits into from
Oct 29, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Oct 26, 2020

This PR creates the @material-ui/unstyled package and moves the SliderUnstyled to this package. The new package is currently added as a dependency in the @material-ui/lab and used inside the SliderStyled component.

It is based on #23264

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2020

Details of bundle changes

Generated by 🚫 dangerJS against d62e6a9

@mnajdova mnajdova requested a review from eps1lon October 26, 2020 15:34
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

What do you think of making @material-ui/unstyled a dependency of @material-ui/core? This was we don't have to duplicate the utils but move them?

"react-is": "^16.8.0"
},
"devDependencies": {
"@material-ui/types": "^5.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Dev dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is dev dependency

packages/material-ui-unstyled/src/Slider/Slider.test.js Outdated Show resolved Hide resolved
}

/**
* @deprecated Not used in this library.
Copy link
Member

Choose a reason for hiding this comment

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

I have added https://trello.com/c/V43vShGf/2455-remove-deprecated-in-typescript so we can have the peace of mind that we won't forget about it in the future.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2020
@eps1lon
Copy link
Member

eps1lon commented Oct 27, 2020

One potential problem we have currently is, we have two components now by the same name @material-ui/unstyled/Slider and @material-ui/core/Slider, which is making problems currently with the docs:api generation.

Every complaint I hear about docs:api always sounds like this is a purely technical discussion and you just battling with code. How does this look from a user perspective in the documentation? The API nav list is currently flat so we can't just put two Slider in there. Naming it "SliderUnstyled" in the nav and then naming it something different in the package is also confusing. I'm also not convinced by "/unstyled/Slider" in the nav since that probably only makes sense to maintainers. I think it makes sense to keep separate names following the discoverability argumentation of "avoid default exports".

So we should setup the documentation for styled and unstyled components first (API only as an MVP) before we complain about docs:api again. This came up in the last meeting and I think we wanted to experiment with a second nav menu that switches between styled and unstyled. More navigation menus is always a bit problematic since you add more navigation "dimensions" but I'd like to see the docs for that first.

To summarize: docs:api doesn't cause problems (I haven't seen any so far). It only highlights unsolved issues.

@mnajdova
Copy link
Member Author

mnajdova commented Oct 27, 2020

Every complaint I hear about docs:api always sounds like this is a purely technical discussion and you just battling with code. How does this look from a user perspective in the documentation? The API nav list is currently flat so we can't just put two Slider in there. Naming it "SliderUnstyled" in the nav and then naming it something different in the package is also confusing. I'm also not convinced by "/unstyled/Slider" in the nav since that probably only makes sense to maintainers. I think it makes sense to keep separate names following the discoverability argumentation of "avoid default exports".

I am not saying we have problems with the docs:api, I said

One potential problem we have currently is, we have two components now by the same name @material-ui/unstyled/Slider and @material-ui/core/Slider

and this is causing problems with docs:api. It's basically what both you and Olivier said. Based on the comments so far on the PR, I will rename the components to SliderUnstyled.

To summarize: docs:api doesn't cause problems (I haven't seen any so far). It only highlights unsolved issues.

Agree, that's why we will rename the component to SliderUnstyled

This PR is just creating the unstyled package and moves the SliderUnstyled there, it is not handling any docs updates, so once we do that, we can revisit this.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 27, 2020
@mnajdova
Copy link
Member Author

@eps1lon seems like in the end we do have a problem with docs:api. After moving the SliderUnstyled to the unstyled package we are loosing all types in the styled Slider that comes from here: https://github.com/mui-org/material-ui/pull/23270/files#diff-d91c63f968bb62cfc91964bceca1d79999f75fcce872ec242a01708c1b1f3b3bL29 What could be the problem here?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 27, 2020

What could be the problem here?

@mnajdova I would start the investigation with: https://github.com/mui-org/material-ui/blob/beb9b0c23011ae53962bb12b9568af5efbb7819f/package.json#L11

The script only looks for components in the core and lab packages.

@mnajdova
Copy link
Member Author

@mnajdova I would start the investigation with:

material-ui/package.json

Line 11 in beb9b0c

"docs:api:build": "cross-env BABEL_ENV=test __NEXT_EXPORT_TRAILING_SLASH=true babel-node --extensions ".tsx,.ts,.js" ./docs/scripts/buildApi.ts ./docs/pages/api-docs ./packages/material-ui/src ./packages/material-ui-lab/src",
The script only looks for components in the core and lab packages.

@oliviertassinari thanks for the suggestion. I've already updated this scripts as well as the script for the yarn proptypes but maybe I need to change the order of which the packages are resolved. Will test this now.

@mnajdova
Copy link
Member Author

Found the issue with the docs generation. In the generatePropstypes.ts script we needed to change the path to the unstyledFile to contain the new unstyled package.

@mnajdova
Copy link
Member Author

What do you think of making @material-ui/unstyled a dependency of @material-ui/core? This was we don't have to duplicate the utils but move them?

@oliviertassinari I've merge the #23264 PR here, so we don't need to add the @material-ui/unstyled as dependency of @material-ui/core. Will add it in my next PR when I'll replace the core Slider with the SliderStyled.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Going to lookup over the types after the merge. It's not viable to review in this PR. In the future, please keep PRs focused on one task. Here: move files.

@mnajdova
Copy link
Member Author

Going to lookup over the types after the merge. It's not viable to review in this PR. In the future, please keep PRs focused on one task. Here: move files.

The PR is based on #23264 that's why it contains that many files, will rebase once that one is merged, plan to merge it soon (I've replaced there to just move the files .js and .d.ts). Will open after this one separate PRs for converting the utilities one by one to typescript.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 28, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 28, 2020
@mnajdova mnajdova merged commit c13deae into mui:next Oct 29, 2020
@zannager zannager added component: slider This is the name of the generic UI component, not the React module! package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants