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

MudIcon: Fix icon color when disabled #8869

Merged
merged 3 commits into from
May 4, 2024

Conversation

dennisrahmen
Copy link
Contributor

@dennisrahmen dennisrahmen commented May 3, 2024

Description

This is a continuation of #7245 which aimed at fixing the color of a menu item that has a custom color set which then overrides the disabled color when the menu item is disabled.

In the mentioned pull request I fixed it by not propagating the custom color to the icon when the component is disabled and with that fixed it for this one component.

But since there are multiple components that use MudIcon this propagation logic would have to be added to each component so I opted to add a 'Disabled' property to the 'MudIcon' instead and propagated the disabled state from the components to the icon.

I checked all components for the use of MudIcon, especially MudTreeView and MudList since those components use multiple icons for the list items and the expansion/collapse.

List disabled with this pr:
image

List disabled before this pr:
image

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)

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 breaking change bug Something does not work as intended/expected PR: needs review labels May 3, 2024
@dennisrahmen
Copy link
Contributor Author

@henon @JonBunator @ScarletKuro
You guys were involved in the first PR regarding this topic, I think this is a more elegant and better suited fix than the single component approach I implemented in the old PR.

I hope this is what you had in mind otherwise I am open for feedback.

Copy link

codecov bot commented May 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.12%. Comparing base (28bc599) to head (fe12f72).
Report is 143 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8869      +/-   ##
==========================================
+ Coverage   89.82%   90.12%   +0.29%     
==========================================
  Files         412      421       +9     
  Lines       11878    12304     +426     
  Branches     2364     2431      +67     
==========================================
+ Hits        10670    11089     +419     
+ Misses        681      666      -15     
- Partials      527      549      +22     

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

@henon
Copy link
Collaborator

henon commented May 4, 2024

[...] so I opted to add a 'Disabled' property to the 'MudIcon' instead [...]

Good idea! Thanks for that

Copy link
Member

@JonBunator JonBunator left a comment

Choose a reason for hiding this comment

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

Looks good

@dennisrahmen
Copy link
Contributor Author

Fixed the parts @ScarletKuro pointed out. Should be ready now.

Sorry about the tagging in the commit message 😅

@henon
Copy link
Collaborator

henon commented May 4, 2024

@dennisrahmen please run dotnet format and push again

@henon henon merged commit 0f37ec5 into MudBlazor:dev May 4, 2024
4 checks passed
@henon
Copy link
Collaborator

henon commented May 4, 2024

Thanks Dennis!

@dennisrahmen dennisrahmen deleted the fix-disabled-icon-color branch May 4, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something does not work as intended/expected PR: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants