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

MudButton: fix focus style button when open dialog #8575

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

LiZzeira
Copy link
Contributor

@LiZzeira LiZzeira commented Apr 5, 2024

Description

The style file for the MudButton components has been updated to include the :focus and :focus-visible pseudo-classes, applying them to the respective buttons.

How Has This Been Tested?

I made a style modification to ensure that the focus on the button is correctly applied when the dialog opens.

This modification addresses several issues, such as:
#4283
#8451

Types 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)

Before:

Dialog.-.MudBlazor.-.Google.Chrome.2024-04-04.22-42-50.mp4

After:
https://github.com/MudBlazor/MudBlazor/assets/79289665/e0609b70-9899-48c0-90de-61f2d585b468
https://github.com/MudBlazor/MudBlazor/assets/79289665/6877c9d1-dd11-496f-bdbf-9cd41498c8b5
https://github.com/MudBlazor/MudBlazor/assets/79289665/07386ced-3695-427b-bacd-10e03e15cabd

Checklist:

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

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Apr 5, 2024
@LiZzeira LiZzeira mentioned this pull request Apr 5, 2024
6 tasks
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.77%. Comparing base (e44b9f3) to head (7147839).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8575      +/-   ##
==========================================
+ Coverage   89.75%   89.77%   +0.02%     
==========================================
  Files         411      411              
  Lines       11817    11817              
  Branches     2362     2362              
==========================================
+ Hits        10606    10609       +3     
+ Misses        683      681       -2     
+ Partials      528      527       -1     

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

@danielchalmers
Copy link
Contributor

Just fyi your sound is on in the clips

@danielchalmers danielchalmers self-requested a review April 5, 2024 02:01
@LiZzeira
Copy link
Contributor Author

LiZzeira commented Apr 5, 2024

lol
Sorry, I hadn't seen it but it's okay

@danielchalmers
Copy link
Contributor

Nice work, again. Thanks for looking into this.

I do wonder why focus-visible is used alone in so many controls though? Must be a reason. @Garderoben may know.
https://github.com/search?q=repo%3AMudBlazor%2FMudBlazor%20focus-visible&type=code

@henon
Copy link
Collaborator

henon commented Apr 5, 2024

I want @Garderoben to double check this to be sure.

@LiZzeira
Copy link
Contributor Author

LiZzeira commented Apr 6, 2024

When using the :focus-visible pseudo-class, the applied style will only be visible if the focused element is via keyboard, such as when pressing the "Tab" key to toggle between buttons.

@Garderoben Garderoben merged commit 42b3803 into MudBlazor:dev Apr 8, 2024
5 checks passed
@danielchalmers
Copy link
Contributor

@LiZzeira Do you think we should be looking at any of the other components that use focus-visible, or is the button enough? https://github.com/search?q=repo%3AMudBlazor%2FMudBlazor%20focus-visible&type=code

@LiZzeira
Copy link
Contributor Author

LiZzeira commented Apr 9, 2024

@danielchalmers I'll conduct some tests on these additional components, and if needed, I'll update and push a new PR

danielchalmers added a commit to danielchalmers/MudBlazor that referenced this pull request Apr 15, 2024
Fixes a regression of MudBlazor#8256 caused by MudBlazor#8575

Not perfect but probably the best with the tools we have.
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

5 participants