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

[website] List Tree View features on the Pricing page #42328

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented May 22, 2024

Follow up on #42100

I did some cleaning in the code.
I struggle to understand some choices in the code of this page. Why do we have 5 listings (one for the JSX Element of the name, of for each plan and yet a 5th one to actually render the row)? This make the action of adding a new item super tedious...

Preview: https://deploy-preview-42328--material-ui.netlify.app/pricing/

Problem

  • We have a doc for the Simple Tree View and a doc to the Rich Tree View. To which pages / doc sections should I link in the Pricing table?

@flaviendelangle flaviendelangle self-assigned this May 22, 2024
@flaviendelangle flaviendelangle added component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! website Pages that are not documentation-related, marketing-focused. labels May 22, 2024
@@ -452,8 +454,8 @@ const rowHeaders: Record<string, React.ReactNode> = {
'data-grid/column-autosizing': (
<ColumnHead label="Column autosizing" href="/x/react-data-grid/column-dimensions/#autosizing" />
),
'data-grid/column-reorder': (
<ColumnHead label="Column reorder" href="/x/react-data-grid/column-ordering/" />
'data-grid/column-reordering': (
Copy link
Member Author

Choose a reason for hiding this comment

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

We have "Row reordering" below

@mui-bot
Copy link

mui-bot commented May 22, 2024

Netlify deploy preview

https://deploy-preview-42328--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 245e01b

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

We have a doc for the Simple Tree View and a doc to the Rich Tree View. To which pages / doc sections should I link in the Pricing table?

This could be asign that the docs organization should be inverted. Group per feature, before per component type 🤔

'data-grid/column-reorder': (
<ColumnHead label="Column reorder" href="/x/react-data-grid/column-ordering/" />
'data-grid/column-reordering': (
<ColumnHead label="Column reordering" href="/x/react-data-grid/column-ordering/" />
Copy link
Member

Choose a reason for hiding this comment

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

To match, no?

SCR-20240524-coia

https://mui.com/x/react-data-grid/column-ordering

Suggested change
<ColumnHead label="Column reordering" href="/x/react-data-grid/column-ordering/" />
<ColumnHead label="Column ordering" href="/x/react-data-grid/column-ordering/" />

Comment on lines +631 to +632
'tree-view/item-virtualization': <ColumnHead label="Item virtualization" />,
'tree-view/item-reordering': <ColumnHead label="Item reordering" />,
Copy link
Member

Choose a reason for hiding this comment

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

This might be simple enough, feels almost clearer

Suggested change
'tree-view/item-virtualization': <ColumnHead label="Item virtualization" />,
'tree-view/item-reordering': <ColumnHead label="Item reordering" />,
'tree-view/item-virtualization': <ColumnHead label="Virtualization" />,
'tree-view/item-reordering': <ColumnHead label="Reordering" />,

),
'tree-view/multi-item-selection': (
<ColumnHead
label="Multi item selection"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clearer

Suggested change
label="Multi item selection"
label="Multi selection"

@@ -601,6 +603,33 @@ const rowHeaders: Record<string, React.ReactNode> = {
'charts/filter-interaction': <ColumnHead label="Row filtering" />,
'charts/selection-interaction': <ColumnHead label="Range selection" />,
'tree-view/tree-view': <ColumnHead label="Tree View" href="/x/react-tree-view/" />,
'tree-view/item-selection': (
<ColumnHead label="Item selection" href="/x/react-tree-view/rich-tree-view/selection/" />
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clearer

Suggested change
<ColumnHead label="Item selection" href="/x/react-tree-view/rich-tree-view/selection/" />
<ColumnHead label="Single selection" href="/x/react-tree-view/rich-tree-view/selection/" />

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we update the grid equivalent feature?

Copy link
Member

Choose a reason for hiding this comment

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

In my view, the data grid, needs to differentiate between row and cell selection, but the pricing page stills uses the same terminology as in the docs pages.

I would see the pricing page always matching with what the docs use, it's one less thing to have to think about. So I think "Should we update the grid equivalent feature?" is truly about asking:

SCR-20240610-upme SCR-20240611-bahj

should stay like this, or become:

SCR-20240610-uprx SCR-20240611-bamp

I personally feel that it works well as it's today in the docs.

@flaviendelangle
Copy link
Member Author

This could be asign that the docs organization should be inverted. Group per feature, before per component type 🤔

Both component do not always behave the same.
So the logic for us was to avoid docs with a lot of "IF YOU ARE ON X THEN DO Y, OTHERWISE DO Z".
Basically, to have the doc as readable as possible, even if it costs some duplication.

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

oliviertassinari commented Jun 10, 2024

We have a doc for the Simple Tree View and a doc to the Rich Tree View. To which pages / doc sections should I link in the Pricing table?

Both component do not always behave the same.

@flaviendelangle Ah right, so <RichTreeView> isn't as simple as <TreeView><TreeItem> composed together and avoid code repetition for developers? Because otherwise, it could be documented like in https://mui.com/material-ui/react-text-field/#components.

Overall, I would propose to be consistent, we have a similar problem with composition for charts, Material UI vs. Base UI + Pigment CSS. Maybe it could be like this:

  • Link/show to the higher-level API first. If people can get away with this, great
  • In the higher-level API pages, reference the lower-level API so that once people hit a wall, have them have an escape path.

For example in https://mui.com/x/react-charts/bars/. I imagine we will create docs for how to recreate the <BarChart> with the lower-level component API cc @alexfauquette

Now, the downside of showing the higher-level API first, and the lower-level API second is that people might stop at the higher-level API, but turned-off and not look further enough to find the lower-level API. But I don't know, the opposite failure mode feels as bad.

It relates a bit to a discussion we had with @colmtuite & @danilo-leal during React Conf about either Material UI v7 should drop the <TextField> API and be pure composition, matching Base UI like API, or be like today, or provide both.

@flaviendelangle
Copy link
Member Author

isn't as simple as composed together and avoid code repetition for developers?

It is not
The SimpleTreeView is an improved version of the component that comes from the core. It has several limitations due to how items are handled, for instance if you disable an item on mount, it flickers because it's the Tree View which stores this information and during the 1st render the item did not have the time to register itself. Collapsed items are, by definition, not rendered so their state is lost, etc...

We could probably heavily re-work the SimpleTreeView component in order to have it fully work with all features and have RichTreeView be a wrapper on top of it, but it's not the road we took. SimpleTreeView is a component meant for basic use cases, with a few items and no complex data manipulation, whereas RichTreeView should be compatible with virtualization, re-ordering, lazy-loading etc... RichTreeView is IMHO not comparable to what the Base UI component are doing, it's closer from the DataGrid in essence, so I don't think the ongoing composition discussions really apply here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants