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-ui][button] Fix sticky hover media query issue on mobile #37467

Merged
merged 10 commits into from
Oct 30, 2023
Merged

[joy-ui][button] Fix sticky hover media query issue on mobile #37467

merged 10 commits into from
Oct 30, 2023

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Jun 1, 2023

Resolves sticky hover effect on mobile devices

Iterate on #36583


This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

Sorry, something went wrong.

Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@mui-bot
Copy link

mui-bot commented Jun 1, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 4ec2e22

@danilo-leal danilo-leal changed the title MUI-36583B - [base][button] hover media query issue on mobile [base][button] Fix sticky hover media query issue on mobile Jun 1, 2023
@danilo-leal danilo-leal added the package: base-ui Specific to @mui/base label Jun 1, 2023
@gitstart gitstart marked this pull request as ready for review June 7, 2023 16:50
@zannager zannager requested a review from siriwatknp June 8, 2023 08:00
gitstart and others added 2 commits June 9, 2023 02:33
Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 20, 2023
Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 27, 2023
@gitstart
Copy link
Contributor Author

@siriwatknp Gentle reminder about this PR

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2023
Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 9, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 13, 2023

On issues like this, I think that the best course of action is, one component at a time:

  1. Iterate on the Joy UI component. For example here fixing the bug in [base-ui][joy-ui] Hover styles persist after clicking on an element on mobile #36583 and handling https://www.notion.so/mui-org/Olivier-design-review-on-Joy-Design-3ada9a7bcfa44b9fab1fe5032dfb33bb. Once we feel great about it, or that we have done enough move to the next step.
  2. Trash the Base UI demo implementation, copy the Joy UI one, but keep it free of all the overhead that comes with allowing developers to customize the components without owning the source.
  3. As new bugs are found in Base UI or Joy UI, copy the fixes in both places, the look and feel should be identical.

cc @mui/joy-ui

In this case, I feel like fixing Base UI demos that are meant to be reference implementation is wasted time since we will trash all the code to redo it based on Joy UI. So we could focus this PR on Joy UI.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work package: joy-ui Specific to @mui/joy and removed package: base-ui Specific to @mui/base labels Aug 13, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 15, 2023
@lithdew
Copy link

lithdew commented Sep 5, 2023

Might there be any updates on this? I'd like to make sure hover effects on mobile are properly disabled for an app I'm working on. Happy to also assist with breaking this PR up into smaller PR's.

Co-authored-by: seunexplicit <48022904+seunexplicit@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 22, 2023
@gitstart
Copy link
Contributor Author

@oliviertassinari we have done the implementation on just the Joy UI library for your review, apologies for the delay. cc @lithdew

@oliviertassinari oliviertassinari changed the title [base][button] Fix sticky hover media query issue on mobile [joy-ui][button] Fix sticky hover media query issue on mobile Sep 24, 2023
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.

👍 Thanks for the fix and sorry for the delay!

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.

👍 Thanks for the fix and sorry for the delay!

@siriwatknp
Copy link
Member

@gitstart Can you pull the latest master? I cannot push to your branch.

image

@gitstart
Copy link
Contributor Author

@gitstart Can you pull the latest master? I cannot push to your branch.

image

The branch has been updated, thanks.

@mnajdova
Copy link
Member

cc @siriwatknp anything else blocking this one from being merged?

@siriwatknp siriwatknp merged commit a2fdd9c into mui:master Oct 30, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants