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

Button: Don't apply :focus effect on mobile so it's not sticky #8709

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

danielchalmers
Copy link
Contributor

@danielchalmers danielchalmers commented Apr 15, 2024

Description

Fixes a regression of #8256 caused by #8575.
See it in action by tapping the button using dev tools (not click).

Not the best solution but it's actively causing a problem and I'm really not sure how else to do it as simple as this.

How Has This Been Tested?

visually

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Before: tapping a button would keep the tap effect visible indefinitely, like the old way.
After: tapping the button only applies the effect while the tap is held, like after the sticky PR.
See the original sticky PR for examples.

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

Fixes a regression of MudBlazor#8256 caused by MudBlazor#8575

Not perfect but probably the best with the tools we have.
@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (28bc599) to head (c0f110d).
Report is 56 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8709      +/-   ##
==========================================
+ Coverage   89.82%   90.08%   +0.25%     
==========================================
  Files         412      418       +6     
  Lines       11878    12048     +170     
  Branches     2364     2365       +1     
==========================================
+ Hits        10670    10854     +184     
+ Misses        681      659      -22     
- Partials      527      535       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielchalmers
Copy link
Contributor Author

@LiZzeira is this ok?

@LiZzeira
Copy link
Contributor

From what I understand, the button remains highlighted because it has active focus on it, is that correct?

@danielchalmers
Copy link
Contributor Author

From what I understand, the button remains highlighted because it has active focus on it, is that correct?

@LiZzeira It does, but the issue is that the focus remains until you click somewhere else on mobile, which means you see the click style indefinitely, which we tried to avoid previously

@henon henon merged commit 3ffa21e into MudBlazor:dev Apr 17, 2024
5 checks passed
@henon
Copy link
Collaborator

henon commented Apr 17, 2024

Thanks @danielchalmers

@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 20, 2024

@LiZzeira It seems like this also affects desktop. Can we get the dialog focus fix without making buttons keep the focus style indefinitely after being clicked?

To reproduce: Go to the button page on the dev docs and click a button. Notice how the click effect remains until you click somewhere else.

#8767 (comment)

@danielchalmers danielchalmers deleted the button-fix-sticky-regression branch April 20, 2024 03:30
danielchalmers added a commit to danielchalmers/MudBlazor that referenced this pull request Apr 20, 2024
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants