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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Accordion] Horizontal padding #20580

Closed
2 tasks done
esseswann opened this issue Apr 16, 2020 · 3 comments 路 Fixed by #20586
Closed
2 tasks done

[Accordion] Horizontal padding #20580

esseswann opened this issue Apr 16, 2020 · 3 comments 路 Fixed by #20586
Labels
component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@esseswann
Copy link
Contributor

esseswann commented Apr 16, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

Expansion panel has 24px padding, while list items have 16px. I am not sure if it corresponds to the spec, but it makes you add extra elements for unification

image

Expected Behavior 馃

Elements should have unified horizontal padding

Your Environment 馃寧

Tech Version
Material-UI v4.9.9
React 16.13.1
Browser Chrome 80.0.3987.163
@oliviertassinari oliviertassinari added component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. labels Apr 16, 2020
@oliviertassinari
Copy link
Member

@esseswann The Material Design specification does no longer covers the expansion panel component. This gives us more freedom around the actual look.

Regarding your request, I have looked at Bootstrap, they use the style of the Card Header. So in our case, I think that it would make sense to do the same and to move forward with your request.

diff --git a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
index 7ed48db1a..b61281403 100644
--- a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
+++ b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
@@ -17,7 +17,7 @@ export const styles = (theme) => {
       display: 'flex',
       minHeight: 8 * 6,
       transition: theme.transitions.create(['min-height', 'background-color'], transition),
-      padding: theme.spacing(0, 3),
+      padding: theme.spacing(0, 2),
       '&:hover:not($disabled)': {
         cursor: 'pointer',
       },

Do you want to submit a pull request? :)


On a side note, I think that we should rename the component in v5 ExpansionPanel -> Accordion. The pros:

@esseswann
Copy link
Contributor Author

esseswann commented Apr 16, 2020

Do you want to submit a pull request? :)

Can do, should it be from some v5 branch or 4.x?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 16, 2020

We don't have a v5 branch yet. master is perfect.

@esseswann esseswann changed the title Expansion panel horizontal padding [ExpansionPanel] Horizontal padding Apr 16, 2020
@oliviertassinari oliviertassinari changed the title [ExpansionPanel] Horizontal padding [Accordion] Horizontal padding Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: accordion This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants