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] Allows to customize menu with any icon #36408

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Mar 3, 2023

Allows to user to chose any icon (tested in mui-x here: mui/mui-x#8148)

image

@mui-bot
Copy link

mui-bot commented Mar 3, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against f564142

@zannager zannager added the docs Improvements or additions to the documentation label Mar 3, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 3, 2023

The visual design of the icon looks fair.

Regarding the API, I'm not sure, it feels like a leaky abstraction of the docs-infra. We are now bundling the charts icon for all the products, and we have to update the docs-infra for using a new nonstandard icon. Supporting this could make sense:

import type { MuiPage } from '@mui/monorepo/docs/src/MuiPage';
import TableViewRoundedIcon from '@mui/icons-material/TableViewRounded';

const pages: MuiPage[] = [
  {
    pathname: '/x/introduction-group',
    title: 'Introduction',
    icon: 'DescriptionIcon', // Standard
    children: [
      { pathname: `/x/introduction`, title: 'Overview' },
      { pathname: `/x/introduction/installation` },
      { pathname: `/x/introduction/licensing` },
      { pathname: `/x/introduction/support` },
      { pathname: `/x/introduction/roadmap` },
    ],
  },
  {
    pathname: '/x/react-data-grid-group',
    title: 'Data Grid',
    icon: TableViewRoundedIcon, // Custom
    children: [

@oliviertassinari oliviertassinari added component: charts scope: docs-infra Specific to the docs-infra product and removed docs Improvements or additions to the documentation labels Mar 3, 2023
@alexfauquette alexfauquette changed the title [docs] Add icon for the chats section of MUI X [docs] Allows to customize menu with any icon Mar 17, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 31, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 7, 2023

#36789 is another reason why to make the icons configurable.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 7, 2023
import pagesApi from 'docs/data/base/pagesApi';

const pages = [
{
pathname: '/base/getting-started',
icon: 'DescriptionIcon',
icon: ArticleRoundedIcon,
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid that this could lead to inconsistency if someone forgets to change the icon on other pages (Material UI, Joy UI, and Base UI follow the same structure). I think it makes more sense for the core pages to reuse the same icons.

Copy link
Member

@flaviendelangle flaviendelangle Apr 13, 2023

Choose a reason for hiding this comment

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

Could we export the "standard page icons" from a doc file ?
Something like

import { DescriptionIcon } from '../some/endpoint/doc-pages-icons';

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved to a solution where AppNavIcons.ts exports the object of icons available. and pages use this object.

For core pages, it means the following diff

- icons: "DescriptionIcon",
+ icons: standardNavIcons.DescriptionIcon,

So you still have a centralized list of standard icons. It's simpler to modify thanks to TS autocomplete, and others can still have access to those icons

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Nice initiative! I have one comment related to sharing icons for the core.

@alexfauquette alexfauquette merged commit 62713bc into mui:master Apr 20, 2023
@oliviertassinari oliviertassinari added new feature New feature or request and removed component: charts labels Apr 23, 2023
@oliviertassinari
Copy link
Member

There is a warning in the console:

Screenshot 2023-04-23 at 15 03 20

Maybe to discourage the use of the string API:

diff --git a/docs/src/modules/components/AppNavDrawerItem.js b/docs/src/modules/components/AppNavDrawerItem.js
index b673cbfe92..ead7c9aac5 100644
--- a/docs/src/modules/components/AppNavDrawerItem.js
+++ b/docs/src/modules/components/AppNavDrawerItem.js
@@ -285,7 +285,7 @@ AppNavDrawerItem.propTypes = {
   comingSoon: PropTypes.bool,
   depth: PropTypes.number.isRequired,
   href: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
-  icon: PropTypes.string,
+  icon: PropTypes.elementType,
   legacy: PropTypes.bool,
   linkProps: PropTypes.object,
   newFeature: PropTypes.bool,

or if we want to support both types

diff --git a/docs/src/modules/components/AppNavDrawerItem.js b/docs/src/modules/components/AppNavDrawerItem.js
index b673cbfe92..0806dcd5c8 100644
--- a/docs/src/modules/components/AppNavDrawerItem.js
+++ b/docs/src/modules/components/AppNavDrawerItem.js
@@ -285,7 +285,7 @@ AppNavDrawerItem.propTypes = {
   comingSoon: PropTypes.bool,
   depth: PropTypes.number.isRequired,
   href: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
-  icon: PropTypes.string,
+  icon: PropTypes.oneOfType([PropTypes.string, PropTypes.elementType]),
   legacy: PropTypes.bool,
   linkProps: PropTypes.object,
   newFeature: PropTypes.bool,

alexfauquette added a commit to alexfauquette/material-ui that referenced this pull request Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants