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

XWIKI-22342: A change in @navbar-height ruins vertical align of items in navbar #3804

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Jan 16, 2025

Jira URL

https://jira.xwiki.org/browse/XWIKI-22342

Changes

Description

  • Finished updating the notification count display to allow for a more resilient styling.
  • Removed the IE8 fix, we don't support it anymore and it messes with our other image styles to break the user avatar size when the navbar width is enough...
  • Added logic for navbar items to fill up the height
  • Centered properly the avatar with a flex display.

Clarifications

  • Security: CSS is safe. No use of user input in the changes.
  • Performance/sustainability: The JQuery lines are ran only once per page (if there's at least one notification). We removed a few lines of CSS but added a few more. It comes with a very small performance cost.
  • Accessibility: The only change to the DOM is the addition of a non semantic element (explicitely defined as only for style).
  • Backward Compatibility: The changes in CSS could break some customizations. However this is not something critical, and AFAIU, some customizations will just not receive the improvements, it should not regress the style. E.g. L88 of actions-menu.less assume some sort of content in the UIX. If the UIX doesn't follow one of the patterns found in XS, they won't get the full improvement. The Notification button DOM changes could break some customizations retrieving the number of notifications through the DOM. I think we can assume that's not many customizations and this is something we can update.
  • Automated tests and quality in general (SonarQube checks): Checked sonarlint on NotificationsDisplayerUIX.xml, nothing to report.
  • Localization: No addition or changes on existing translations.

Screenshots & Video

2025-01-16.14-38-02.mp4

In order to test multiple values quickly, instead of setting a defined height (like I'd advise for an actual use), I set the navbar height as a fraction of the screen (half). This way, redimensionning the screen allowed me to quickly change the value of the navbar height.

We can see that the navbar, with the changes proposed here the icons stay properly in the center. The only issue left is on very low width, the avatar seems to behave a bit weirdly.

Executed Tests

Manual tests, as seen above.
We can assume the changes in CSS don't break automated tests.
The test model for the updated notification count should not break because of the changes:

In order to check this, I first built the change with mvn clean install -f xwiki-platform-core/xwiki-platform-notifications/xwiki-platform-notifications-ui; mvn clean install -f xwiki-platform-distribution and then ran the mention tests mvn clean install -f xwiki-platform-core/xwiki-platform-mentions/xwiki-platform-mentions-test/xwiki-platform-mentions-test-docker -Pdocker. These tests use the Notification count (they count mention notifications). The build and tests passed successfully

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.10.X (pretty safe changes)

… in navbar

* Updated the notification count display to allow for a more resilient styling. --WIP
… in navbar

* Finished updating the notification count display to allow for a more resilient styling.
* Removed the IE8 fix, we don't support it anymore and it messes with our other image styles to break the user avatar size when the navbar width is enough...
… in navbar

* Added logic for navbar items to fill up the height
* Centered properly the avatar with a flex display.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant