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.

Sign up for GitHub

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
Open

XWIKI-22674: Search facets item count misalignment #3758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ...platform-search-solr/xwiki-platform-search-solr-ui/src/main/resources/Main/SolrSearch.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,6 @@ a.search-result-highlightAll:after {
color: $theme.titleColor;
cursor: pointer;
line-height: 1.4em;
margin: 0 .2em;
display: flex;
justify-content: space-between;
position: relative;
Expand Down Expand Up @@ -758,12 +757,16 @@ a.search-result-highlightAll:after {
.search-facet-body li {
display: flex;
flex-wrap: wrap;
padding: .1em .2em;
padding: .1em 0;
}

.search-facet .search-facet-header .facet-toggler, button.facet-value-toggler {
background: transparent;
transition: background-color .2s ease-in-out;
/* We overwrite the default xs-button style to make sure the icon is properly aligned on the right of the element.
By default there is a 5px padding on each side. */
padding-right: 0;
padding-left: 10px;
}

.search-facet .search-facet-header .facet-toggler:active, button.facet-value-toggler:active {
Expand Down Expand Up @@ -845,8 +848,12 @@ body.content.preference-underlining-only-inline-links #xwikicontent .search-face
}

.search-facet-body .itemCount {
padding: .1em 0;
padding: .2em;
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

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?

align-self: center;
}

@media (max-width: 768px) {
Expand Down