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

[material-ui][Button] Add demo for file upload #38786

Merged
merged 15 commits into from
Sep 4, 2023
Merged

[material-ui][Button] Add demo for file upload #38786

merged 15 commits into from
Sep 4, 2023

Conversation

anle9650
Copy link
Contributor

@anle9650 anle9650 commented Sep 3, 2023 •
edited by danilo-leal
Loading

@mui-bot
Copy link

mui-bot commented Sep 3, 2023 •
edited
Loading

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 5bbd11a

@anle9650 anle9650 marked this pull request as draft September 3, 2023 05:59
@anle9650 anle9650 marked this pull request as ready for review September 3, 2023 06:23
@danilo-leal danilo-leal changed the title [Button] Add demo for file upload [material-ui][Button] Add demo for file upload Sep 4, 2023
@danilo-leal danilo-leal added docs Improvements or additions to the documentation component: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material component: upload This is the name of the generic UI component, not the React module! labels Sep 4, 2023
danilo-leal
danilo-leal approved these changes Sep 4, 2023
View reviewed changes
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.

Looking good; thanks for contributing! 🤙
Left a suggestion that simplifies the demo code a bit.

docs/data/material/components/buttons/InputFileUpload.js Outdated Show resolved Hide resolved
@danilo-leal danilo-leal merged commit df4562a into mui:master Sep 4, 2023
oliviertassinari
oliviertassinari reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@anle9650 Thanks, great demo!

@michaldudak Comparing https://mui.com/joy-ui/react-button/#file-upload to https://deploy-preview-38786--material-ui.netlify.app/material-ui/react-button/#file-upload is interesting. Joy UI button based on Base UI needs a role={null} while Material UI doesn't. A bug in Base UI Button component?

docs/data/material/components/buttons/InputFileUpload.tsx
Comment on lines +6 to +16
const VisuallyHiddenInput = styled('input')`
clip: rect(0 0 0 0);
clip-path: inset(50%);
height: 1px;
overflow: hidden;
position: absolute;
bottom: 0;
left: 0;
white-space: nowrap;
width: 1px;
`;
Copy link
Member

@oliviertassinari oliviertassinari Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I know, the standard is for all the demos should use the JavaScript object syntax in Base UI, Joy UI,
Material UI. I imagine it's because 1. the source are written this way, so easier for developers to move between source and demos, and 2. it's more typesafe.

Can we update these?

docs/data/material/components/buttons/InputFileUpload.tsx
component="label"
variant="contained"
startIcon={<CloudUploadIcon />}
href="#file-upload"
Copy link
Member

@oliviertassinari oliviertassinari Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is wrong as far as I know:

Suggested change
href="#file-upload"
Screenshot 2023-09-05 at 00 09 21

https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-38786--material-ui.netlify.app%2Fmaterial-ui%2Freact-button%2F

@danilo-leal danilo-leal mentioned this pull request Sep 5, 2023
Merged
3 tasks
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
@anle9650 @mnajdova
[material-ui][Button] Add demo for file upload (mui#38786)
68541cb
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
@anle9650 @mnajdova
[material-ui][Button] Add demo for file upload (mui#38786)
c4bcb03
@michaldudak
Copy link
Member

Comparing https://mui.com/joy-ui/react-button/#file-upload to https://deploy-preview-38786--material-ui.netlify.app/material-ui/react-button/#file-upload is interesting. Joy UI button based on Base UI needs a role={null} while Material UI doesn't. A bug in Base UI Button component?

@oliviertassinari Base UI button adds the role="button" if the component is not a native button to make the buttons accessible. In this case, we explicitly don't want the button behavior on the label, as it already has other a11y semantics, so IMO, it makes sense we need to unset the role attribute.

In Material UI, if we create a <Button component="span" />, it will not be accessible.

xcode-it pushed a commit to xcode-it/material-ui that referenced this pull request Sep 11, 2023
@anle9650 @xcode-it
[material-ui][Button] Add demo for file upload (mui#38786)
871f86d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@danilo-leal danilo-leal danilo-leal approved these changes

Assignees
No one assigned
Labels
component: button This is the name of the generic UI component, not the React module! component: upload This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[material-ui][Button] Add demo for file upload
5 participants
@anle9650 @mui-bot @michaldudak @oliviertassinari @danilo-leal