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

[Joy] Use icon inside a Button #30803

Merged
merged 11 commits into from
Feb 1, 2022
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jan 27, 2022

Preview:

Playground: https://deploy-preview-30803--material-ui.netlify.app/experiments/joy/components/
Demo: https://deploy-preview-30803--material-ui.netlify.app/experiments/joy/button/

Design highlights:

  • add Button CSS variables (naming can be discussed later). The great benefit about using CSS variables is that we don't need to use pseudo selectors like in @mui/material/Button. Also, the customization experience is a lot better because developers only need to change the value without writing the whole stylesheet for every size.

    • --Button-minHeight: to make the button's height explicit and predictable for different sizes.
    • --Button-gutter: represents the padding-left & right of the button to be used to calculate icon margin.
    • --Button-iconOffsetStep: the negative margin of the start/end icon
    • --Button-gap: the space between start/end icon and the text
  • add startIcon & endIcon prop to the Button

  • (it makes theming harder) add square prop to the Button (to create IconButton). I think this is a lot easier than creating another component, aka IconButton, and maintaining both of these buttons. Here is my thought process.

    • I want a button with an icon inside, so I write this <Button><PlusIcon /></Button>
    • I see it on the screen that the button is a rectangle but I want the shape to be a square.
    • I expect to add one more prop like <Button square><PlusIcon /></Button> to make the width & height equally and if I want it to be rounded, I would add sx or style overrides depending on the whole design look & feel.

    You might ask, why not shape="square | circular". I have tried that out and it did not work well because shape: square and shape: circular target different CSS properties or more than 1 property depending on how you interpreted them.

    • square: target the width to equal to the height
    • circular: target the border-radius

    It is hard to predict the UI when 1 prop targets different CSS properties. However, this is just from my experience of using it and I might be wrong. We will be able to test this out with the UI examples and revisit it again.

What's next?

  • Add IconButton component
  • We will need to think about how to document CSS variables.
  • CSS variables naming for using inside the components. Should it be
    • --MuiButton-minHeight or
    • --joy-Button-minHeight or
    • --Button-minHeight
  • Add loading prop to Button

@mui-bot
Copy link

mui-bot commented Jan 27, 2022

Details of bundle changes

@mui/joy: parsed: +1.33% , gzip: +1.03%

Generated by 🚫 dangerJS against 3225a55

@siriwatknp siriwatknp marked this pull request as draft January 27, 2022 07:11
@siriwatknp
Copy link
Member Author

The change detected in Argos is due to the reduction of the height for the outlined variant.

@siriwatknp siriwatknp marked this pull request as ready for review January 27, 2022 12:15
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.

Nice, great work as usual Jun!

Not to get too lost in the naming discussion now, but I think that the proposed square prop would be better if it was icon. Instead of thinking of it as something that changes the styles of the Button, I think it's more that changes the type of Button used.

@danilo-leal danilo-leal added the package: joy-ui Specific to @mui/joy label Jan 28, 2022
@siriwatknp
Copy link
Member Author

Nice, great work as usual Jun!

Not to get too lost in the naming discussion now, but I think that the proposed square prop would be better if it was icon. Instead of thinking of it as something that changes the styles of the Button, I think it's more that changes the type of Button used.

Good point, I experience a downside when testing the square prop (or icon if you'd like) with theming.

// By doing this, the `square` or `icon` does not work because the style overrides has more priority
styleOverrides: {
  MuiButton: {
    root: ({ ownerState }) => ({
      '--Button-gutter': '1rem',
    })
  }
}

// So I have to do this instead
styleOverrides: {
  MuiButton: {
    root: ({ ownerState }) => ({
       // takes a while to figure out why the prop `square` does not work.
       ...(!ownerState.square && {
          '--Button-gutter': '1rem',
        }),
    })
  }
}

However, if we expose IconButton component the styleOverrides will be much simpler in more intuitive like this:

styleOverrides: {
  MuiButton: {
    root: ({ ownerState }) => ({
       '--Button-gutter': '1rem', // adjust the left, right padding
    })
  },
  MuiIconButton: {
    root: ({ }) => ({
      '--IconButton-padding': '0.25rem', // adjust all padding sides
    })
  }
}

@siriwatknp
Copy link
Member Author

@danilo-leal I have removed the square prop for now. I think it is good to go (the API is the same as the Material, except the component CSS variables)

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.

Nice, it's making sense so far. We might as well stress test this a bit more with example UIs, but so far so good!

@siriwatknp siriwatknp merged commit 4c79daf into mui:master Feb 1, 2022
wladimirguerra pushed a commit to wladimirguerra/material-ui that referenced this pull request Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants