fix(side-panel): fullscreen button visibility#1547
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses a visibility issue with the fullscreen button in the side panel. The change increases the specificity of the CSS selector for the fullscreen button, ensuring that its display property, which is controlled by a CSS variable for visibility, is not overridden by more general theme styles. The fix is correct and well-targeted. I have no further comments.
|
Documentation. Coverage Reports: |
|
Can we add a unit test to prevent future regressions 🤔 ? I'm unsure how good the setup around unit testing in the side-panel is - if it's a big hassle I'm fine with making testing improvements as follow up task. |
7ff70fc to
896c3a8
Compare
ljanner
left a comment
There was a problem hiding this comment.
@dauriamarco that's some serious test coverage you added! Well done 👏 🚀
Only a few minor details
| .btn-icon.fullscreen-button { | ||
| display: var(--fullscreen-button-display, inline-flex); | ||
| } |
There was a problem hiding this comment.
| .btn-icon.fullscreen-button { | |
| display: var(--fullscreen-button-display, inline-flex); | |
| } | |
| .fullscreen-button { | |
| display: var(--fullscreen-button-display, inline-flex) !important; // stylelint-disable-line declaration-no-important | |
| } |
The reason why the fix with the additional .btn-icon specifier works is because this rule has higher specificity than the .btn class which applies display: flex. If anybody tries to figure out when what display value is set they need to play the game of specificity.
I generally think putting !important here would be a really valid choice, because it makes it more clear that it is an intentional value.
There was a problem hiding this comment.
Thank you @ljanner :) I understand your point here 👍 and I would also like to now what @spike-rabbit thinks about this before proceeding.
projects/element-ng/side-panel/si-side-panel-content.component.html
Outdated
Show resolved
Hide resolved
projects/element-ng/side-panel/si-side-panel-content.component.html
Outdated
Show resolved
Hide resolved
896c3a8 to
7d4f204
Compare
Closes #1529.