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

BUGFIX: Prevent override of height for icons #3914

Open
wants to merge 1 commit into
base: 8.4
Choose a base branch
from

Conversation

markusguenther
Copy link
Member

The icons have multiple classes. From Font Awesome, we obtain the custom replacement class neos-svg-inline—fa, which sets a height of 1rem. However, we define the selectBoxHeaderWithSearchInput__icon class and set the height to 40px via a variable. The issue arises because the order in which these classes are applied determines the winning height. In Neos 8.4, the 40px height prevails, while in Neos 9.0, the 1rem height takes precedence. Consequently, the icons appear misaligned.

To address this issue, we implement an adjustment that ensures that when an element possesses both of these classes, the height of 40px from our custom class prevails.

Related: #3882

The icons have multiple classes. From Font Awesome, we obtain the custom replacement class neos-svg-inline—fa, which sets a height of 1rem. However, we define the selectBoxHeaderWithSearchInput__icon class and set the height to 40px via a variable. The issue arises because the order in which these classes are applied determines the winning height. In Neos 8.4, the 40px height prevails, while in Neos 9.0, the 1rem height takes precedence. Consequently, the icons appear misaligned.

To address this issue, we implement an adjustment that ensures that when an element possesses both of these classes, the height of 40px from our custom class prevails.

Related: neos#3882
@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.4 labels Feb 4, 2025
@markusguenther
Copy link
Member Author

markusguenther commented Feb 4, 2025

With the Upgrade in #3801 some Icons were misaligned.

Before:
Screenshot 2025-02-04 at 18 36 15

After:
Screenshot 2025-02-04 at 18 34 34

@markusguenther
Copy link
Member Author

The acceptance tests for Firefox seem to be a bit wonky at the moment.

@mhsdesign
Copy link
Member

Thanks for fixing this

And i tested it and the override works ... thanks! Now im wondering why this change is targeting 8.4 when the bug only appears in 9.0?

In Neos 8.4, the 40px height prevails, while in Neos 9.0, the 1rem height takes precedence.

In Neos 8.4 the class is indeed ordered differently in the build output:

.neos-svg-inline--fa{display:inline-block;font-size:inherit;height:1em;
.mAesQG_selectBoxHeaderWithSearchInput__icon{height:40px;width:1em}

Instead of neos-svg-inline--fa being last like in 9.0:

.mAesQG_selectBoxHeaderWithSearchInput__icon{height:40px;width:1em}
.neos-svg-inline--fa{display:var(--fa-display, inline-block);height:1em;

Also i noticed that the "width:1em" part is new with the font awesome upgrade too and wonder how that comes into play.

https://github.com/neos/neos-ui/pull/3801/files#diff-487adff98ba8cb5993634e088dd98a9904b1860ed6c58d4f788f1895822c5793


overall its pretty mean to have the styles somehow wind up in a different order than expected and i also expect that explains: #3882 and #3861
... maybe we can find out the reason for that?

@markusguenther
Copy link
Member Author

Hi @mhsdesign

the neos-svg-inline--fa brings the height of 1rem and depending on the ordering of the classes the height comes from the neos-svg-inline--fa or the selectBoxHeaderWithSearchInput__icon class. I target 8.4 as the upgrade was intoduced here and when somehow the ordering changes, the bug will be the same.

So it is more prevention to make clear that our class will win :)

Copy link
Contributor

@pKallert pKallert left a comment

Choose a reason for hiding this comment

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

Tested it out locally and looks fine

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Cannot reproduce as previously mentioned by Marc, but ok 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.4 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants