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-22674: Search facets item count misalignment #3758

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

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Dec 12, 2024

Jira URL

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

Changes

Description

  • Fixed the alignment
  • Added a highlight of the counters
  • reduced the size of counter text

Clarifications

  • There are a few changes that are not necessary for XWIKI-22674. However, they are closely related and a first step on the way to implementation of the improvements described in XWIKI-22676.

Screenshots & Video

After the PR:
Screenshot from 2024-12-12 11-44-17

Here is a quick video demo of how the elements interact and helping to see how they align with each other.

2024-12-12.11-50-06.mp4

Executed Tests

None except manual tests shown above, style changes in a SSX only almost never have any impact on tests, especially those which don't involve hiding some elements like this one.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • 16.10.X (no reason not to, relatively low scope and low criticity changes)

* Fixed the alignment
* Added a highlight of the counters
* reduced the size of counter text
@Sereza7
Copy link
Contributor Author

Sereza7 commented Dec 12, 2024

@tkrieck Can you validate that the changes proposed here are okay with what you reported https://jira.xwiki.org/browse/XWIKI-22674 and proposed in https://jira.xwiki.org/browse/XWIKI-22676 ? As a precision, this is not a full implementation of your UI proposal, it's just a first step to update the style of the counts .

Thanks!

@tkrieck
Copy link
Contributor

tkrieck commented Dec 12, 2024

@tkrieck Can you validate that the changes proposed here are okay with what you reported https://jira.xwiki.org/browse/XWIKI-22674 and proposed in https://jira.xwiki.org/browse/XWIKI-22676 ? As a precision, this is not a full implementation of your UI proposal, it's just a first step to update the style of the counts .

Thanks!

Hi @Sereza7 thank you for taking this issue. Yes it's perfect, I know it's not a full implementation, but the misalignment was pretty important to give the UI a polished look.

margin-left: auto;
background-color: $theme.highlightColor;
border-radius: 7px; /* Same value as @border-radius-base from Flamingo. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use a variable from a theme here instead of a hard-coded value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a CSS SSX, so we can't use LESS variables directly:
image

We would need to migrate it to a LESS SSX. In order to do so, we'd need to remove all the references to old color theme variables (which are in the format $theme.<something>) and find an equivalent in the LESS variables we have. It's a bit beyond the scope of this PR, and I'm not really eager to move things to LESS knowing that we'll soon want to get back to regular CSS...

Note that this situation (with the exact same comment) has already been encountered and solved like this:
https://github.com/search?q=repo%3Axwiki%2Fxwiki-platform+Same+value+as+%40border-radius-base+from+Flamingo.&type=code

margin-left: auto;
background-color: $theme.highlightColor;
border-radius: 7px; /* Same value as @border-radius-base from Flamingo. */
font-size: 0.8em;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question for the font-size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it's the .small mixin and not a variable. We don't have an alternative for CSS yet, so I just went with a hardcoded value.
Note that even in LESS this value gets hardcoded often ^^'
https://github.com/search?q=repo%3Axwiki%2Fxwiki-platform+font-size%3A&type=code

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we start adding a common mechanism asap instead of hard-coding them more and more?
iirc we agreed to start using CSS variables, can't we introduce one for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manuelleduc

I'd rather introduce them all at once so that we don't duplicate things or have inconsistent ways to provide them.

If that's okay with you, I think it'd make sense to put this PR on hold (as a draft) as long as XWIKI-22667 is not done. This ticket could be too high priority to wait for this though.

WDYT?

Copy link
Contributor

@tkrieck tkrieck left a comment

Choose a reason for hiding this comment

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

As stated in another comment, the changes fix the main issue from the Jira. I won't comment on the code though, as Manual already did it.

All is fine by me, thank you! 👍

@Sereza7
Copy link
Contributor Author

Sereza7 commented Dec 13, 2024

All comments have been addressed. This PR is ready for more discussion or a merge.

@Sereza7 Sereza7 requested a review from manuelleduc December 13, 2024 10:04
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.

3 participants