-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix a regression with theming of settings menu icons #38246
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
@VicDeo Cool! Can you add a changelog? Fix looks good. |
@micbar added |
changelog/unreleased/38246
Outdated
@@ -0,0 +1,6 @@ | |||
Bugfix: Fix a regression with theming of settings menu icons | |||
|
|||
Default icons were loaded into the settings menu instead of icons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default icons were loaded into the settings menu instead of icons | |
Default icons were loaded instead of the overrides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Kudos, SonarCloud Quality Gate passed! |
Description
Firstly URLGenerator is instantiated when user session is loaded and the theme is not available yet on this stage.
So an empty theme is cached into the URLGenerator instance for a while.
Partially reverts #37867
Related Issue
Motivation and Context
Fixes resolution of images added into the settings navigation by app-themes
How Has This Been Tested?
settings/img/users.svg
)Expected
users.svg from theme is loaded
Actual
users.svg from core is loaded.
Types of changes
Checklist: