-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[accordion] Keep active Accordion in viewport #44002
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-44002--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
Accordion
in viewportAccordion
in viewport
Accordion
in viewport@@ -73,6 +73,7 @@ const AccordionRoot = styled(Paper, { | |||
}, | |||
}, | |||
[`&.${accordionClasses.expanded}`]: { | |||
overflowAnchor: 'auto', // keep active accordion in the viewport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have any impact? I see no change.
Same question for overflowAnchor: 'none', // Keep the same scrolling position
it seems that we don't need this anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have any impact? I see no change.
@oliviertassinari it does
A) (before) all accordions have overflowAnchor: 'none'
before-none-for_all.mp4
B) (after) all accordions have overflowAnchor: 'none'
except expanded one
after-auto_for_expanded.mp4
C) no overflowAnchor
at all
no_overflow_anchor_at_all.mp4
Same question for
overflowAnchor: 'none', // Keep the same scrolling position
it seems that we don't need this anymore.
There is a slight difference in B
and C
(after clicking second accordion)
In B
you can see the first accordion
In C
you can not see the first accordion
This was my proposal, I am okay with removing overflowAnchor: 'none'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw joy ui's accordion and it does not have overflowAnchor
ig we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B) looks better, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you like me to make any changes to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yash49 Check the "After fix" CodeSandbox in the description. Minimize the window and notice that the second accordion doesn't fully come into view when expanded.
@ZeeshanTamboli did you open the site in new tab? or you only tried it in the integrated preview of CodeSandbox? For me, I can randomly produce the issue in the integrated preview. Maybe CodeSandbox(iframe) is doing something, but I am not sure; I could be wrong. |
closes #34379
Related to #22292
Preview: https://deploy-preview-44002--material-ui.netlify.app/material-ui/react-accordion/#only-one-expanded-at-a-time
Before: https://codesandbox.io/p/sandbox/distracted-sanne-rre74j
After: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-wvkgqv