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

[pickers] Migrate PickersToolbarButton to emotion #25989

Merged
merged 24 commits into from
May 10, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 26, 2021

BREAKING CHANGE

Remove customization of MuiPickersToolbarButton.

This was an internal component for which we can no longer support customization. We'll rework the customization story in the next weeks during which this component might be removed.

One of #24405

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 26, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 7af8843

@oliviertassinari oliviertassinari added the component: pickers This is the name of the generic UI component, not the React module! label Apr 26, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 28, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 28, 2021
@oliviertassinari oliviertassinari changed the title [Pickers] Migrate PickersToolbarButton to emotion [pickers] Migrate PickersToolbarButton to emotion Apr 29, 2021
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've just moved the declaration for the classes to be before they are used. Apart from that looks good 👍

@mnajdova
Copy link
Member

@eps1lon can we merge this one as the first of the pickers migration? It would establish at least an initial template, like where to use generics, how to use the types etc.

@mnajdova mnajdova requested a review from eps1lon April 30, 2021 14:55
@mnajdova
Copy link
Member

mnajdova commented May 3, 2021

Got it, yeah I think that it makes sense. I’ll remove the class name then too, and if there is use case for styling these internal components, we can add both the className and the theme overrides in the future.

mnajdova and others added 3 commits May 4, 2021 00:34
@eps1lon
Copy link
Member

eps1lon commented May 4, 2021

@mnajdova Doesn't the new approach work as well?

Let's try to follow fundamental approach of not adding any features we don't need. All under the assumption that these components are no longer public. In case the recent weeks weren't clear to core members: Pickers is under a major internal revamp because the prior implementation is not usable for implementors. So we need to re-examine how we approach these things.

Once pickers are in a useable state we can actually examine what we need and not just throw existing tools at it simply because they exist. It didn't work before so let's try a different approach.

@eps1lon eps1lon requested a review from mnajdova May 4, 2021 07:47
mnajdova
mnajdova previously approved these changes May 4, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented May 4, 2021

@eps1lon OK, and to clarify my previous point:

What we have learned so far is that developers needs to be able to customize all the elements of our components, either by swapping large chunks or by being very chirurgicale, only changing one element. This can happen with hook only API, slot components, slot class name, or sx props on the composed element.

I personally don't see the value of removing global class names, I expect that developers will complain as soon as we do, and we will add them back. One thing that might be great when we add them back is that de treat these internal components as public extension of the public one. Meaning, like TablePagination's Action works, it's a private component, but still public from a customization perspective under the same component name.

Actually, if we take a look at the other components, the only elements in the DOM that we consider private from a customization perspective might be the grid-window for virtulization on the data grid and the nested div the Collapse (that we want to remove). I can't think of any other in the library. For instance, SwipeableDrawer area is accessible with a prop style, NotchedOutline is accessible with fieldset/legend element. But maybe there are others I forgot.

@mnajdova
Copy link
Member

mnajdova commented May 4, 2021

I am ok with using the styled() utility here, the issue I see is, in the next component, when we will have styles dependent on props, we will need to either, use custom shouldForwardProps and pass props, or pass props via styleProps as with the other components, and then just for the sake of not using experimentalStyled() we will be bringing new rules and contributors will be confused what to use.

I understand that this is a private component, but I rather not break up the consistency we have. This is a lab component, so if we break something we can easily fix it without issues, so I don't want to push too much with any details on the implementation, as long as we make some progress.

I think we overcomplicated in the end with the picker components, especially when you are saying they may go away. Maybe we should have continued the effort, without blocking the migration for over a week.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 4, 2021

@mnajdova 👍 for consistency (solving the same problems the same way) and solving one problem at the time.

@eps1lon
Copy link
Member

eps1lon commented May 5, 2021

Maybe we should have continued the effort, without blocking the migration for over a week.

And maybe we should have waited with pickers migration so that we don't end up in this mess. But I'm done giving ground on this issue. Pickers are in a terrible shape and at every point in time I was overruled and in the end I turned out to be right.

Maybe we should have just keep it simple instead of cramming all these features into a project under heavy construction that need to be learned first. So this will be the migration going forward. We keep it simple because we are not capable of dealing with all these abstractions at once.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 5, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 5, 2021
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 agree that we should have probably waited. The only thing I would change on the PR is the import to experimentalStyled, just for the sake of keeping it consistent with the rest of the components. We are not gaining much with using styled() directly from styled-engine. And seeing these final changes, I think that the migration would be trivial and I don't see any blockers at this moment.

@mnajdova mnajdova dismissed their stale review May 5, 2021 17:03

Wrong styled() util used

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've updated to use experimentalStyled for leveraging the default theme. I believe we are ready to merge.

@mnajdova
Copy link
Member

I am merging this one. Updated to use experimentalStyled and skipped the sx prop (I will add skipSx on the other internal components in the core too). @siriwatknp would be great if you can update the rest of the open PRs on the internal pickers components to follow this template.

@mnajdova mnajdova merged commit 651f8ff into mui:next May 10, 2021
@siriwatknp
Copy link
Member Author

I am merging this one. Updated to use experimentalStyled and skipped the sx prop (I will add skipSx on the other internal components in the core too). @siriwatknp would be great if you can update the rest of the open PRs on the internal pickers components to follow this template.

Noted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants