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

[docs] Replace react-inspector with custom TreeView implementation #17662

Merged
merged 13 commits into from
Nov 12, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 2, 2019

Use our own TreeView instead of react-inspector for https://material-ui.com/customization/default-theme/ (i.e. dogfeeding).

Current bugs:

  • keyboard navigation between items doesn't work (collapsing works but not up/down arrow). Any idea why @joshwooding ?

TODO:

  • dark/light mode themes use syntax highlighting theme
  • expand all
  • expandPaths
  • [ ] property preview for root object (feature cut, not that useful)
  • hide expand icon when object has no properties

Feedback regarding TreeView API:

  • naming of defaultCollapseIcon might be confusing: Does this mean the icon that is displayed when we can collapse or is this the icon which is displayed when the item is collapse i.e. when we can expand (the latter one is the case). I would suggest swapping the names so that you don't have to write defaultCollapseIcon={<ExpandIcon />} which adds some cognitive overload ("I have to use the icon indicating expand for collapseIcon")
  • end icon is using terminology unknown to me. While leaf icon is just as "jargony" it's at least Math/CS jargon.

@eps1lon eps1lon 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! labels Oct 2, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 2, 2019

No bundle size changes comparing 4bfc2e4...eb375bb

Generated by 🚫 dangerJS against eb375bb

@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from 499e58d to 438da84 Compare October 2, 2019 08:36
@joshwooding
Copy link
Member

Was trying to look into it but after building locally. I get a random error saying next-server can't find trim-right. It doesn't even look like the file reported uses trim-right either.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 3, 2019

Was trying to look into it but after building locally. I get a random error saying next-server can't find trim-right. It doesn't even look like the file reported uses trim-right either.

We had some weird issues with netlify reporting the same error (we don't use trim-right anywhere anymore). Try clearing your node_modules and then docs/.next if that didn't resolve the issue.

@joshwooding
Copy link
Member

So this is due to a limitation with the TreeView. https://github.com/mui-org/material-ui/blob/master/packages/material-ui-lab/src/TreeView/TreeView.js#L45 It looks for nodeId on its immediate child. ObjectTreeItem has nodeId but passes a different nodeId to TreeItem which breaks things.

@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from d4309dd to 7874c2e Compare November 4, 2019 15:00
@eps1lon
Copy link
Member Author

eps1lon commented Nov 4, 2019

So this is due to a limitation with the TreeView. /packages/material-ui-lab/src/TreeView/TreeView.js@master#L45 It looks for nodeId on its immediate child. ObjectTreeItem has nodeId but passes a different nodeId to TreeItem which breaks things.

I completely missed this response, thanks. I'll finish this PR over the week. Current version should have working keyboard navigation.

@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from 7874c2e to 62f4e5d Compare November 6, 2019 15:34
@eps1lon eps1lon marked this pull request as ready for review November 7, 2019 14:56
@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from 62f4e5d to 3566b22 Compare November 8, 2019 08:26
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.

It's so cool to see our tree view used here 😍. I have noticed the following:

  1. I think that we have an indent issue. Compare these two cases:
  • Capture d’écran 2019-11-11 à 13 45 58
    Capture d’écran 2019-11-11 à 13 46 05
  • Capture d’écran 2019-11-11 à 13 46 58
    Capture d’écran 2019-11-11 à 13 47 08

What do you think of adding an indent of the same size as the expand icon, for the leaf case?

  1. We don't have enough contrast on the number type to reach the AA level. However; given that it seems to use the same style as the code formatting, I suspect this change https://github.com/mui-org/material-ui/pull/18283/files#r344488449 will fix the problem.

Capture d’écran 2019-11-11 à 13 47 35

@eps1lon

This comment has been minimized.

@eps1lon

This comment has been minimized.

@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from f97ef62 to 60e408e Compare November 11, 2019 15:32
Does not affect contrast-ratio
@eps1lon eps1lon force-pushed the docs/theme-inspector-treeview branch from 566eb39 to dcf43d8 Compare November 11, 2019 18:06
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! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants