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.

Sign up for GitHub

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

Jump to bottom

[Tree View] Polish the default design & revise the simple version pages #11529

Merged
merged 28 commits into from
Feb 7, 2024
Merged

[Tree View] Polish the default design & revise the simple version pages #11529

merged 28 commits into from
Feb 7, 2024

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented Dec 28, 2023 •
edited
Loading

As the title says! This PR hopefully closes #11528, and it adds a small coat of polish to the default Tree View look & feel as well as a general revision to the Overview page, applying the latest page structure as described in mui/material-ui#39702. It was a hard one, particularly the disabled section and explaining the ContentProps stuff (which felt too obscure).

https://deploy-preview-11529--material-ui-x.netlify.app/x/react-tree-view/

@danilo-leal danilo-leal added docs Improvements or additions to the documentation component: tree view TreeView, TreeItem. 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 labels Dec 28, 2023
@danilo-leal danilo-leal self-assigned this Dec 28, 2023
@mui-bot
Copy link

mui-bot commented Dec 28, 2023 •
edited
Loading

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

Updated pages:

Generated by 🚫 dangerJS against 72b1879

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

github-actions bot commented Jan 2, 2024

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

@flaviendelangle
Copy link
Member

It was a hard one, particularly the disabled section and explaining the ContentProps stuff (which felt too obscure).

I'd like to rework this API which is not consistent with the rest of the X components (they are using slots).

@samuelsycamore
Copy link
Contributor

I'm getting ready to do a line-by-line review, but from the jump it feels odd to me that this is the "Overview" page for the Tree View. All of the other products use the Overview page pattern to introduce the product and its key features at a high level—closer to marketing than documentation, really. Maybe that pattern isn't as useful in this case since there isn't as much to document, but it feels strange and out of step with the rest of the X docs nonetheless—it throws off my expectations as a reader. I'm not sure what the solution should be. Is there a plan to eventually expand this content into multiple pages? Or will it remain more like a Core-style component demo document?

@flaviendelangle
Copy link
Member

Is there a plan to eventually expand this content into multiple pages?

The doc has been split into several pages in #11059 which has just been merged
I first migrated the doc from the core without modifying it and now we are enriching it and restructuring it 👍

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 2, 2024
@danilo-leal danilo-leal changed the title [Tree View] Polish the default design & revise the Overview page [Tree View] Polish the default design & revise the simple version pages Jan 2, 2024
@danilo-leal
Copy link
Contributor Author

@samuelsycamore — that makes sense! I merged this PR with master and pulled all the changes from Flavien's PR linked above. For the sake of not exploding this PR's scope even more, I have constrained myself to tackle only the Simple Tree View pages, also because they mostly house the content that was in the all-encompassing Overview page before. :) We can definitely work on revising the Rich-related pages afterward! 🤙

@flaviendelangle
Copy link
Member

We can definitely work on revising the Rich-related pages afterward!

A lot of it it purely duplicate from SimpleTreeView so it should be an easy follow up 👍

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

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

@flaviendelangle
Copy link
Member

😆 Polishing the Tree View doc is an hard task with Git conflict.
Respect @danilo-leal 🙈

@danilo-leal
Copy link
Contributor Author

🤣 I'm considering closing this PR and starting fresh from scratch, heh

@flaviendelangle
Copy link
Member

The doc structure should be pretty stable now 👍
We are mostly adding new doc sections

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2024
@danilo-leal
Copy link
Contributor Author

I think this PR is good-to-go for a review whenever y'all can! 👍

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

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 26, 2024
samuelsycamore
samuelsycamore reviewed Jan 29, 2024
View reviewed changes
docs/data/tree-view/overview/overview.md Outdated Show resolved Hide resolved
docs/data/tree-view/overview/overview.md Outdated
Comment on lines 20 to 21
- `@mui/x-tree-view/TreeView`: for simpler use cases.
- `@mui/x-tree-view/RichTreeView`: for more complex and larger uses of the Tree View component.
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than saying "this one is simple" and "this one is complex", maybe we should provide some clarifying details? (How do I know if my use case is complex or large enough to warrant the Rich Tree View?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think we can cut this part altogether given the following headings explain exactly that!

docs/data/tree-view/overview/overview.md Outdated Show resolved Hide resolved
docs/data/tree-view/overview/overview.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/customization/customization.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/items/items.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/items/items.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/items/items.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/items/items.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/selection/selection.md Outdated Show resolved Hide resolved
noraleonte
noraleonte reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Thank you for this effort! 🎉

Leaving some small suggestions for improvements 😄

docs/data/tree-view/overview/overview.md Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/selection/selection.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/expansion/expansion.md Outdated Show resolved Hide resolved
docs/data/tree-view/simple-tree-view/selection/selection.md Show resolved Hide resolved
@danilo-leal
Copy link
Contributor Author

Little bump on this one! :)

noraleonte
noraleonte approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Looking great! Thanks for picking this up! 🎉 🍰

@danilo-leal danilo-leal merged commit c5cc556 into mui:next Feb 7, 2024
17 checks passed
@danilo-leal danilo-leal deleted the polish-tree-view branch February 7, 2024 11:06
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 7, 2024 •
edited
Loading

Nice 👍

A likely follow-up: #11976.

@danilo-leal danilo-leal mentioned this pull request Feb 7, 2024
Merged
1 task
@danilo-leal
Copy link
Contributor Author

Follow-up PR opened above → #11979

thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
@danilo-leal @thomasmoon
[Tree View] Polish the default design & revise the simple version pag…
d89f9db 
…es (mui#11529)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noraleonte noraleonte noraleonte approved these changes

@samuelsycamore samuelsycamore Awaiting requested review from samuelsycamore

Assignees

@danilo-leal danilo-leal

Labels
breaking change component: tree view TreeView, TreeItem. 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 docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[tree view] Polish the look and feel
6 participants
@danilo-leal @mui-bot @flaviendelangle @samuelsycamore @oliviertassinari @noraleonte