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

[Popper] Move from mui-material to mui-core #28923

Merged
merged 18 commits into from
Oct 18, 2021
Merged

[Popper] Move from mui-material to mui-core #28923

merged 18 commits into from
Oct 18, 2021

Conversation

rebeccahongsf
Copy link
Contributor

@rebeccahongsf rebeccahongsf commented Oct 8, 2021

Moved the Popper component to the unstyled package and updated all references.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 8, 2021

Details of bundle changes

@material-ui/unstyled: parsed: +43.84% , gzip: +43.47%

Generated by 🚫 dangerJS against 5278e11

@rebeccahongsf rebeccahongsf reopened this Oct 8, 2021
@mbrookes mbrookes added the package: base-ui Specific to @mui/base label Oct 9, 2021
@rebeccahongsf
Copy link
Contributor Author

@michaldudak aside from the Azure Devops check that failed (I'm actively trying to figure out why), the PR is ready for first round of feedback/review. Thank you! 😄

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I could notice few things still missing. Great first iteration 👍

packages/mui-material/src/Tooltip/Tooltip.d.ts Outdated Show resolved Hide resolved
packages/mui-material/src/index.d.ts Outdated Show resolved Hide resolved
packages/mui-material/src/index.js Outdated Show resolved Hide resolved
packages/mui-core/src/Popper/Popper.js Show resolved Hide resolved
@rebeccahongsf
Copy link
Contributor Author

rebeccahongsf commented Oct 11, 2021

Huzzah! Thank you @mnajdova and @eps1lon! 🙌 I'll get those changes up in a jiffy.

@rebeccahongsf
Copy link
Contributor Author

Ready for another round of feedback! 😃

Any suggestions on how I might go about troubleshooting the Azure bundle size error? I read within the contribution guidelines that its probably due to the packages or how the documentation was built. 🤔 Perhaps I'm missing a step on building a bundle?

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good to me. The Azure Pipelines are failing with JavaScript heap out of memory, I would ignore it for now.

packages/mui-core/src/Popper/Popper.test.js Outdated Show resolved Hide resolved
packages/mui-lab/src/internal/pickers/PickersPopper.tsx Outdated Show resolved Hide resolved
@eps1lon eps1lon changed the title [Popper]: Move from mui-material to mui-core [Popper] Move from mui-material to mui-core Oct 12, 2021
@eps1lon
Copy link
Member

eps1lon commented Oct 12, 2021

Looks good to me. The Azure Pipelines are failing with JavaScript heap out of memory, I would ignore it for now.

Why? This job is an integral part of the CI pipeline. You can't just ignore parts because they fail. CI isn't just there to affirm you but also to signal mistakes you make. I've heard this a couple of times now from you and it's a concerning sentiment. I don't understand why you say this repeatedly. If you think there's a problem with CI in general, then let's discuss this separately. But constantly saying "ignore CI" in PRs is not a heatly sentiment.

There is definitely something off here that needs to be adressed.

@mnajdova
Copy link
Member

Why? This job is an integral part of the CI pipeline. You can't just ignore parts because they fail. CI isn't just there to affirm you but also to signal mistakes you make. I've heard this a couple of times now from you and it's a concerning sentiment. I don't understand why you say this repeatedly. If you think there's a problem with CI in general, then let's discuss this separately. But constantly saying "ignore CI" in PRs is not a healthy sentiment.

I said, " I would ignore it for now". I meant for this iteration of the review, there are still things that need to be polished, we can focus on it in the end. I am not going to merge the PR if the CI is not green.

@mnajdova
Copy link
Member

Talking about the Azure Pipelines, we should move the condition for the Popper component from: https://github.com/mui-org/material-ui/blob/master/scripts/sizeSnapshot/webpack.config.js#L20 to line 35 (inside the core package) otherwise the entry id would be overridden.

@rebeccahongsf
Copy link
Contributor Author

Thank you @eps1lon and @mnajdova! PR #29005 resolved the bundling issue. I implemented the requested changes. Please do not hesitate to let me know if any other changes are needed. 😃

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.

Looks good. Nice work 👍🏻

@michaldudak michaldudak merged commit 0845873 into mui:master Oct 18, 2021
@michaldudak
Copy link
Member

Congratulations @rebeccahongsf on being the first community contributor to create an unstyled component in @mui/core! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants