XWIKI-24617: Improvements to panel titles and navigation items - #6048
XWIKI-24617: Improvements to panel titles and navigation items#6048tkrieck wants to merge 13 commits into
Conversation
* Changed the default styles for the Navigation panel only * Selected page now have a pseudo-bold applied * Links now spans 100% of the container * Spacing improved between elements
* Changed the default styles Panel titles
| // Panels headings are the same size as the content's h4 headings. | ||
| font-size: @font-size-h4; | ||
| font-size: @font-size-h5; | ||
| font-weight: 600; |
There was a problem hiding this comment.
I surprised to see a hardcoded weight here. I suggest adding a comment explaining why a default value is not relevant.
There was a problem hiding this comment.
It was done this way because #5526 its not merged. I could change it to a variable, but it could stand out a lot with the wrong values that we have currently for the semibold variable.
There was a problem hiding this comment.
#5526 is ready to be merged as well (it's a very simple code change). But if you think it's better/safer, I'll add the TODO here.
| --> | ||
|
|
||
| <xwikidoc version="1.5" reference="Panels.Navigation" locale=""> | ||
| <xwikidoc version="1.7" reference="Panels.Navigation" locale=""> |
There was a problem hiding this comment.
I suggest removing all the changes that are not strictly related to your PR.
the xwikidoc version, and the list of values just below mainly.
There was a problem hiding this comment.
This was done by mvn xar:format command when I exported the page. Should I remove it?
There was a problem hiding this comment.
@tmortagne I'm not sure what's the best practice here
There was a problem hiding this comment.
mvn xar:format definitely did not move the version from 1.5 to 1.7. That's because you exported from a more recent version of XWiki.
I'm not sure what's the best practice here
The best practice is generally to skip any change which has nothing to do with the goal, as with anything else.
There was a problem hiding this comment.
I'll move the version back to 1.5 then. Thanks @tmortagne
| <cache>forbid</cache> | ||
| </property> | ||
| <property type="LargeString"> | ||
| <code>.Navigation { |
There was a problem hiding this comment.
I don't understand why you need to add a new CSS XObject.
Also, this .Navigation selector looks odd.
There was a problem hiding this comment.
The object is necessary to add the CSS code to the page, is there another/better way? The .Navigation is necessary to keep these changes active only on the navigation panel itself. Since the classes changed are from JStree, I didn't want them to affect something outside the main panel (like the breadcrumb page menu)
There was a problem hiding this comment.
ok, that explains the unusual uppercase class name.
- Can we also add a parent selector that would make the selector more specific?
- Is it interesting to add a comment explaining the use of the
Navigationclass
There was a problem hiding this comment.
Can we also add a parent selector that would make the selector more specific?
I'll check what's available to be used. But sure, I don't see why not. It should make the changes even more safe.
* Added parent selector to the navigation class * Added font-weight variable
* Rolled back xwikidoc version
| <size>5</size> | ||
| <unmodifiable>0</unmodifiable> | ||
| <values>action=Action|doc.reference=Document|icon.theme=Icon theme|locale=Language|rendering.defaultsyntax=Default syntax|rendering.restricted=Restricted|rendering.targetsyntax=Target syntax|request.base=Request base URL|request.cookies|request.parameters=Request parameters|request.url=Request URL|request.wiki=Request wiki|user=User|wiki=Wiki</values> | ||
| <values>action=Action|doc.reference=Document|doc.revision|icon.theme=Icon theme|locale=Language|rendering.defaultsyntax=Default syntax|rendering.restricted=Restricted|rendering.targetsyntax=Target syntax|request.base=Request base URL|request.cookies|request.headers|request.parameters=Request parameters|request.remoteAddr|request.session|request.url=Request URL|request.wiki=Request wiki|sheet|user=User|wiki=Wiki</values> |
There was a problem hiding this comment.
I would revert that one too, to keep the diff minimal and to the point.
There was a problem hiding this comment.
Addressed in 55f5015
I was on the fence about this one since I don't really know what all of this is. Probably added in the export process too.
* Rolled back line in navigation.xml
| <cache>forbid</cache> | ||
| </property> | ||
| <property type="LargeString"> | ||
| <code>.panels { /* .panels and .Navigation parents to isolate the rules /* |
There was a problem hiding this comment.
I think all these new CSS rules would be better off defined directly in panels.less, misc.less or tree.less (to be decided rule by rule).
Currently, the combination of no cache and always used make it loaded uncached on all pages, regardless of the presence of the interesting UI element.
There was a problem hiding this comment.
Currently, the combination of no cache and always used make it loaded uncached on all pages, regardless of the presence of the interesting UI element.
I can test it using other caching methods. The ones active right now are probably a leftover from when I was developing it.
I think all these new CSS rules would be better off defined directly in panels.less, misc.less or tree.less (to be decided rule by rule).
IMO changing the .less files makes more sense, and it's easier to implement without messing with page exports, formats, and all that. It would take a bit of time to move the rules and test it but I'm ok with it.
On a side note, this is something that I get confused about a lot in XWiki, when rules go into pages themselves and when they go into the .less files. I run into both ways quite a lot, even some that are partly defined in files and partly defined in pages, so it's hard to evaluate the proper way when doing these implementations.
There was a problem hiding this comment.
If you want we can discuss that in a call. The decision is a mix of legacy stuff and when the CSS rules need to be loaded (ultimately, we aim for loading exactly the CSS rules that are needed to render what's presented to the user, no less, no more).
There was a problem hiding this comment.
When changing the cache policy to anything other than "On this Wiki" the changes stop having any effect (unless when on the navigation page itself). So I'll need to move them over to a .less file like you suggested. Should I change this PR back into a draft?
There was a problem hiding this comment.
Yes, if you change the use value to current page, it's only going to be loaded automatically when visiting the page where the XClass is defined only.
Otherwise, you need to explicitly load the CSS using velocity, as explained in https://www.xwiki.org/xwiki/bin/view/Documentation/DevGuide/Tutorials/SkinExtensionsTutorial/#HTestingtheactualextension
So this behavior is expected.
Anyway, the right approach here is to move the CSS to the less files instead, so this does not impact you here.
+1 to move back to the draft state as the PR won't be merged without some large-ish changes.
There was a problem hiding this comment.
Moved back to draft, thanks for the help
| /* Allows text wrapping in containers and resets native nowrap */ | ||
| ul.jstree-children, | ||
| .jstree-node { | ||
| white-space: normal; |
There was a problem hiding this comment.
Isn't this property already defined in tree.less?
| flex-flow: row wrap; /* Allows sub-lists (ul) to drop down to the next line */ | ||
| align-items: flex-start; | ||
|
|
||
| i.jstree-ocl { |
There was a problem hiding this comment.
I think you can safely avoid using the element name in addition to the class for .jstree-* classes.
This is overly restricting the selector and could lead to breakages if the internal implementation of jstree changes.
| i.jstree-ocl { | |
| .jstree-ocl { |
Fixed comment closing Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>
* Changed parent selector rule Co-authored-by: Manuel Leduc <manuel.leduc@xwiki.com>
* Moved all style rules from navigation.xml to a specific section in panels.less
Jira URL
https://jira.xwiki.org/browse/XWIKI-24617
Changes
Description
Clarifications
Screenshots & Video
Before:
Overview
Nav Panel
After:
Overview
Nav Panel
Executed Tests
Changes were mostly estilistic in nature and should not break any tests. However, I did run the docker tests ont he skin. I had many troubles with it since it didn't pass even on the master branch (so, without these changes). I had to use
-Djacoco.haltOnFailure=falseto make it work. I guess it's not a very good practice, and I accept any help to run it cleanly.mvn clean install -Pquality,integration-tests,docker -pl :xwiki-platform-flamingo-skin-test-docker -Dxwiki.test.ui.browser=firefox -Djacoco.haltOnFailure=falseAlso, manual tests on desktop and mobile
Expected merging strategy
*