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] MultiSectionDigitalClock layout review #15258

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arthurbalduini
Copy link
Member

Extracted from #14397

When working on the PR mentioned above, it was noticed that currently the actionBar causes a stretch on the MultiSectionDigitalClock, causing UI issues:

Currently, we have a slight misalignment of the margins when two or more actions are available

image image

I proposed some changes on the UI on that PR, but it led to discussions so it made sense to discuss it separately, where we can all discuss design changes. The image below illustrates the impacts of the change.

image

Some inputs to the discussions:

  • Maybe in the future new actions can be added and impact the sizing even more
  • Should the sections expand to use the stretched space caused by the actionBar?
  • Should the buttons expand vertically and fill the whole section area ?
  • This discussion might generate a solution that will also close [pickers] Time section scroll bar gets in front of section column #9311

@arthurbalduini arthurbalduini added component: pickers This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Nov 4, 2024
@@ -118,7 +126,7 @@ const PickersLayout = React.forwardRef(function PickersLayout<
>
{isLandscape ? shortcuts : toolbar}
{isLandscape ? toolbar : shortcuts}
<PickersLayoutContentWrapper className={classes.contentWrapper}>
<PickersLayoutContentWrapper ownerState={{ valueType }} className={classes.contentWrapper}>
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in the main PR, the ownerState should comme from usePickerLayout.

But overall I have an issue with this BC because PickersLayoutContentWrapper is public so people will need to pass manually the ownerState and it's not a great DX IMHO.
We really need to rework the whole DX at some point to hide those implementation details.

@mui-bot
Copy link

mui-bot commented Nov 4, 2024

Deploy preview: https://deploy-preview-15258--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 1a5c9d7

@noraleonte
Copy link
Contributor

I think the design changes make sense in this case 👌
Just one small comment: I think it would make sense if the buttons within each section would stretch so that the text is centered 😸

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 5, 2024
Copy link

github-actions bot commented Nov 5, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 21, 2024

Once #15494 is merged, you should be able to just add pickerValueType to PickerOwnerState (I think it's worth adding it there since it's not specific to the layout).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Time section scroll bar gets in front of section column
4 participants