-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Integrate StickyTOC to chameleon as modification component #386
Integrate StickyTOC to chameleon as modification component #386
Conversation
NOTE: This is my first time doing chameleon components, so I'm learning as I go and would need someone to help me test and go review. Thanks! |
I tested and read through the code. Mostly looks good but I have some notes:
|
@note: This fixes deprecated call to Hook::run() as well.
48d2044
to
66ab0a3
Compare
Thanks for reviewing @bawolff.
For this one, I'm still trying to understand what "aside" means. Does it mean "left" or "right" aside? Maybe but I have no issue with setting it as the default instead of "left". Just the ambiguity in the word "aside" for me.
Done!
Done! I've introduced a new |
aside means the TOC is separate from the text on the left side, where 'left' basically means it overlaps text after scrolling. I guess it is just personal preference and doesn't matter too much. |
Yep that looks good. I think we should also not apply the stickytoc setting to things setting sticky but not stickytoc for backwards compatibility. |
It looks like there is also a typo on the script path for sticky.js |
I made a pull request for the two small remaining issues. https://github.com/xSavitar/chameleon/pull/1 |
Thank you @bawolff |
Fix typo in script path and don't apply sticky toc to just sticky
|
||
<row> | ||
<cell> | ||
<modification type="Stickytoc"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this file?
|
||
.stickytoc ul li { | ||
font-size: 16px; | ||
color: #355B84; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My minor concern with using Less is that the rest of the skin uses SCSS. Adding colors into this file means it cannot automatically use SCSS variables. It also seems like Less is used only for nesting purposes here, but it's inconsistent since many of the selectors have multiple classes but don't use nesting.
Not using SCSS is not necessary an issue if this file contains only the minimum structural styling necessary. But I would prefer to not introduce Less in a SCSS project when we're not really using any Less features.
.stickytoc #toc .toctitle h2 { | ||
font-weight: 700; | ||
font-size: 15px; | ||
//color: white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary CSS.
@@ -40,8 +40,23 @@ | |||
}, | |||
"ChameleonEnableExternalLinkIcons": { | |||
"value": false | |||
}, | |||
"ChameleonStickyTOCFloat": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have component config in here. All other components do this in the layout file and I don't see a special reason for this to be an exception.
* @since 1.0 | ||
* @ingroup Skins | ||
*/ | ||
class Stickytoc extends Modification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this should be a Modification. If you look at the other Modifications, they are generic and intended to be used for modifying other components, e.g. making something else sticky or hidden. Specifically, by placing the modification node inside the component node to be affected in the XML.
In this case Stickytoc is not really modifying the parent node (<cell>
in stickytoc.xml
). While it is technically making changes to what is inside MainContent, it does not work in the same way as other Modifications.
I agree that the TOC should go in the bordered sidebar. |
@xSavitar do you think you will find time to update your PR? |
@krabina We have since merged an alternative approach, although it is currently undocumented and needs more testing for custom layouts. On the latest master branch, you can add the <component type="Toc"/> I don't currently have a wiki to show, but here is an early video of it: normal.mp4 |
Looking great! Can you share your layout.xml file? |
@krabina Here's a basic example: <?xml version="1.0" encoding="utf-8"?>
<structure xmlns="https://ProfessionalWiki.github.io/chameleon/schema/3.5/layout.rng">
<component type="NavbarHorizontal" class="sticky-top">
<component type="Logo" position="head"/>
<component type="NavMenu" flatten="navigation"/>
<component type="PageTools" class="flex-row" position="right" hideSelectedNameSpace="yes"/>
<component type="SearchBar" class="order-first order-cmln-0" position="right" buttons="go"/>
<component type="PersonalTools" position="right"/>
</component>
<grid mode="fluid">
<row>
<cell class="d-none d-md-block col-md-3 col-lg-2 border-right">
<component type="Toc"/>
</cell>
<cell class="col-12 col-md-9 col-lg-10">
<component type="SiteNotice"/>
<component type="MainContent"/>
</cell>
</row>
</grid>
<grid class="mb-2 mt-4" mode="fluid">
<row>
<cell>
<component type="NavbarHorizontal" collapsible="no" class="small mb-2" >
<component type="Toolbox" flatten="no" class="dropup"/>
<component type="LangLinks" flatten="no" class="dropup"/>
</component>
</cell>
</row>
<row>
<cell>
<component type="FooterInfo"/>
<component type="FooterPlaces"/>
</cell>
<cell>
<component type="FooterIcons" class="justify-content-end"/>
</cell>
</row>
</grid>
</structure> |
In my first test it looks great and is working. However, the "hide" "show" for the TOC is not working and in the browser console I see this:
|
Another issue: on pages where there is no TOC at all, the space for the TOC should not be wasted. |
What should be shown instead? I personally would not like to expand the content area to the sidebar. |
Oh yes, exactly that. I have a wiki where the customer wants this because of very long content pages. There it is a great feature. But on many other pages, content is shown with different result formats (tables, etc.). There every inch of screen size is precious. I really only need the StickyTOC on the left side if there is a TOC. Since you can deliberatly chose this with TOC NOTOC (and even FORCETOC it would be a great feature to only use the space once a TOC should be there and give it back to the main content of there is no TOC |
BTW: an option position="right" would also be great, since a blank space on the left is much more annoying than a blank space on the right. Tweeki does it like this: https://tweeki.kollabor.at/wiki/How-Tos |
The difficulty here is that the sidebar is defined in the layout, independent of the TOC. So you could also put other things in the sidebar, or even define the sidebar in a completely different way. Having the TOC then take control of a parent element/component is kind of out of scope for the TOC component. But we could perhaps add a class to the tag to indicate that a TOC is present/absent. And then the admin can add custom styling to hide the whole sidebar. |
I observed an interesting effect: If you use the result format "datatables" on a page, it will somehow use the left sidebar as well. This is good, because datatables usually need as much width as they can get, but it is somehow unintended. So maybe this gives a hint on how this could be turned into a feature. |
I've sent #405 to try and improve the styles of the component; I believe this can be closed |
Closing this as superceded by work being done for #390. |
@note: This fixes deprecated call to Hook::run() as well.