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

[material-ui][docs] Revise the Accordion page #40284

Conversation

anle9650
Copy link
Contributor

@anle9650 anle9650 commented Dec 23, 2023

Part of #39952. Adds documentation for accordion's complementary components.

https://deploy-preview-40284--material-ui.netlify.app/material-ui/react-accordion/#introduction

@anle9650 anle9650 changed the title [material-ui][docs][Accordion] document complementary components [material-ui][docs][Accordion] improve documentation for complementary components Dec 23, 2023
@mui-bot
Copy link

mui-bot commented Dec 23, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against d814a5e

@danilo-leal danilo-leal added docs Improvements or additions to the documentation component: accordion This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Dec 23, 2023
@danilo-leal danilo-leal changed the title [material-ui][docs][Accordion] improve documentation for complementary components [material-ui][docs] Revise the Accordion page Dec 25, 2023
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet! Thank you, @anle9650, for kicking this one off! Took advantage of this PR to expand the scope a bit and already revisef the Accordion page as a whole! 👌 Tagging @samuelsycamore for a further content review, too!

(PS: I'm seeing a hydration error that I didn't quite figure out where yet)

@danilo-leal danilo-leal requested review from samuelsycamore and removed request for siriwatknp December 25, 2023 16:40
@anle9650
Copy link
Contributor Author

@danilo-leal nice! I fixed the hydration error in 802eb4a

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

Love to see the improvements here! @danilo-leal do you think we should give this page the full editorial makeover (#35158) or try to limit the scope to #39952?

docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
anle9650 and others added 9 commits December 25, 2023 16:22
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Andy Le <andy.le@ucdenver.edu>
@danilo-leal
Copy link
Contributor

@samuelsycamore, appreciate the review! 🙏 Are you foreseeing any other changes to this PR? I applied the latest template you put together for the Material UI docs on my first edit, and with these edits, I think it's looking pretty good. What do you think?

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

couple more tiny style edits

docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
docs/data/material/components/accordion/accordion.md Outdated Show resolved Hide resolved
@samuelsycamore
Copy link
Contributor

@samuelsycamore, appreciate the review! 🙏 Are you foreseeing any other changes to this PR? I applied the latest template you put together for the Material UI docs on my first edit, and with these edits, I think it's looking pretty good. What do you think?

One thing that's not entirely clear to me is how comprehensive the component demo pages should be, compared with the API reference pages. There are several props that are documented in the API page (like expandIcon, which feels like an important element of the component), but their use isn't explicitly demonstrated on the demo page. Should we try to cover them all here? (My gut says yes, because I think there's a fair number of users who only use the the demo pages.)

@danilo-leal
Copy link
Contributor

Good call—pushed a commit adding a few more sections for other props available to the Accordion!

One that is still broken, though, that I couldn't figure out properly, is the transition demo. Would appreciate some help here! It seems there's a TypeScript issue with passing another transition component using the TransitionComponent pro. Every other demo, from other components, using that prop does it in a slightly different way. I'd love to have something like the Tooltip transition demo; it's simple, and it just works. 😬 Not sure what's going on here — aside from the TS thing, the content does fade out, but the container still stays there... 🤔

@anle9650

This comment was marked as resolved.

@danilo-leal
Copy link
Contributor

Uhm, I'm hesitant to change things on the source code of the Fade component to pull this PR off — maybe there's another way out that I'm just not seeing?

@anle9650
Copy link
Contributor Author

anle9650 commented Dec 27, 2023

@danilo-leal I'm seeing that TransitionComponent.children is optional in Accordion, but is required in Tooltip. Making this prop also required in Accordion fixes the problem...

---TransitionProps & { children?: React.ReactElement<any, any> }
+++TransitionProps & { children: React.ReactElement<any, any> }

Maybe Accordion was supposed to also be included in #29028 updates, but was missed?

@danilo-leal
Copy link
Contributor

danilo-leal commented Dec 27, 2023

Oh, interesting; it does seem to solve the TypeScript issue, but not the uncollapsed Accordion Details panel. Maybe @mnajdova or @DiegoAndai could help us out with this matter? 😬

@DiegoAndai
Copy link
Member

DiegoAndai commented Jan 4, 2024

Hi! I discussed with @mnajdova, and we think that the best path forward is to:

  • Cast the type on the demo for this PR (TransitionComponent={Fade as AccordionProps['TransitionComponent']). This is safe to do, as the Accordion component always provides children to the transition component.
  • We'll deprecate TransitionComponent prop in favor of slots.transition soon. I started to work on adding the deprecations yesterday. So let's get the correct type in slots.transition and not modify TransitionComponent.

Does that make sense?

Regarding this:

aside from the TS thing, the content does fade out, but the container still stays there

By replacing the Collapse animation, we lose the collapse mechanic that changes the container's height. I recommend creating a custom transition for this demo that combines the collapse and fading mechanics.

@danilo-leal
Copy link
Contributor

@DiegoAndai appreciate the guidance! Let me know if the way I did it up there works well — didn't use a custom transition but instead toggled the styles conditionally using the Accordion's expanded state. 🤙

Signed-off-by: Diego Andai <diego@mui.com>
@DiegoAndai
Copy link
Member

@DiegoAndai appreciate the guidance! Let me know if the way I did it up there works well — didn't use a custom transition but instead toggled the styles conditionally using the Accordion's expanded state. 🤙

@danilo-leal yeah! that looks good 😊

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Yay, this is looking good to me! Thanks for kicking it off & working with us throughout @anle9650! 🎉

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

looks great!

@danilo-leal danilo-leal merged commit c3f3e16 into mui:master Jan 5, 2024
19 checks passed
@anle9650
Copy link
Contributor Author

anle9650 commented Jan 8, 2024

Thanks for all your help and feedback on this!

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Co-authored-by: Diego Andai <diego@mui.com>
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! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants