XWIKI-23961: Inconsistencies in the "underline inline links" preference #5136
Conversation
* Fixed the hover state for preference=No * Removed the underlining on the search page entries names * Removed the underlining on the search page version numbers * Removed the underlining from breadcrumbs found in the page content. * Fixed the search filter items being underlined only on XWiki org. * Removed underline from the selectize links that come with icons. * Removed underline for navigation elements.
| & .jstree-xwiki, // Do not underline links in jstrees | ||
| & .xwiki-livedata .cell .displayer-link, // Do not underline links that are in their own livedata cell | ||
| & .xwiki-livedata .cell .breadcrumb // Do not underline links that are in breadcrumbs in livedatas | ||
| & .xwiki-livedata .cell .breadcrumb, // Do not underline links that are in breadcrumbs in livedatas |
There was a problem hiding this comment.
Why keep this one since you added the one below which seems to include it?
There was a problem hiding this comment.
Breadcrumb can also used to represent a space (see screenshot above). In such case, the breadcrumb primary meaning is not to be a navigation element and the specific rule is needed. Here, we discussed and came to the conclusion that the breadcrumbs in this list should not be navs (https://forum.xwiki.org/t/ui-accesibility-breadcrumbs-in-the-document-index/12866).
There was a problem hiding this comment.
Why is the specific rule needed since you added below:
& .breadcrumb,
?
There was a problem hiding this comment.
My bad, I need to remove the specific rule to use this generic one that could be reused by users 👍
Done in d988877
| & .xwiki-livedata .cell .breadcrumb, // Do not underline links that are in breadcrumbs in livedatas | ||
| & .breadcrumb, // The breadcrumb contains enough presentation to distinguish it from neighboring text. | ||
| & nav, & *[role="navigation"] // Navigation nodes are meant to contain anchors. This meaning is most of the time conveyed through the style | ||
| { |
There was a problem hiding this comment.
Also, we weren't highlighting the breadcrumb at the top of pages, what was the rule preventing that and now that you've added the nav exclusion, shouldn't we remove the specific rule for that breadcrumb?
There was a problem hiding this comment.
The breadcrumb at the top of pages is not in the content. The base of the "inline" filter is considering all links in the content are inline and all links outside are not.
| &.search-result-version // The search result version is distinct from neighboring text. | ||
| { | ||
| .not-inline-link(); | ||
| } |
There was a problem hiding this comment.
Generally-speaking I'm really not fond of all these specific lists because they are hardcoding dependencies and creating a bad dependency between the flamingo skin and specific extensions that it has to know. Thus, they don't address the generic case .
It would be much better to have a single class value to prevent highlighting links and modify the various extensions/specific pieces of code to use that rather than group them all here, which doesn't scale.
To give a specific example, &.user-name, &.group-name seems to be related to the userDisplayMacro. This should done in the code of that extension and not here, using some generic class. Similar to what a contrib extension needs to do.
| #set ($attachmentURL = $xwiki.getURL($searchResultReference)) | ||
| #set ($downloadHint = $services.localization.render('core.viewers.attachments.download')) | ||
| <a href="$attachmentURL" title="$escapetool.xml($downloadHint)"> | ||
| $services.icon.renderHTML('attach') |
There was a problem hiding this comment.
Seems you're moving the icon as part of the link, why? Why change that suddenly and in this PR (it seems it's not the topic of this PR to address that).
Thx
There was a problem hiding this comment.
Because we have a rule to not underline links with an image/icon contained in them.
When I looked at this UI, it looked like it should have matched that rule, but because of this slight difference in DOM it wouldn't.
If we don't do this, we need to add .force-underline to these or add an exception.
IMO it's okay to merge this small quality/consistency improvement in here because it avoids creating another workaround.
There was a problem hiding this comment.
Because we have a rule to not underline links with an image/icon contained in them.
Yes, I understood this but I don't think it's a valid reason to make that change. We need to want to make the icon clickable. Is that what we do in general and elsewhere? Is that something we really want? At first sight, it looks a bit weird to me to do that.
vmassol
left a comment
vmassol
left a comment
There was a problem hiding this comment.
See the comment I've left. Thx!
* Removed a repetitive rule
3 participants
Jira URL
https://jira.xwiki.org/browse/XWIKI-23961
Changes
Description
Clarifications
The underlines were changed with a couple methods:
Screenshots & Video
After the PR:
Executed Tests
Manual tests of all the places targeted on my local instance (see screenshots).
Built the changes with
mvn clean install -f xwiki-platform-core/xwiki-platform-web/xwiki-platform-web-war -Pqualityandmvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-resources -Pquality.The tests
mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker/ -Dxwiki.test.ui.wcag=truepassed as expected.Expected merging strategy