-
-
Notifications
You must be signed in to change notification settings - Fork 99
Make Autotoc tab markup compatible with Bootstrap #1468
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
Conversation
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.
Thanks a lot @cihanandac
This LGTM, but I leave the review to @thet and @petschki
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 have tried this out and have found some issues:
editform tabs
- the active tab bottom border isn't transparent anymore
- the tabs have too much horizontal spacing (compared to classic demo site)
new:

original:

table of contents
the table_of_contents
display is now a tabbed display instead of a vertical link list:
f10acde
to
937db88
Compare
@petschki Thank you so much for the review. I really appreciated the suggestions.
|
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.
@cihanandac thanks for taking care of the Bootstrap tabs here, but I'd really like to cut this a bit more.
You already use the Bootstrap Tab
library and the ul -> li -> a
markup with its css classes correctly. So I think we should try to remove our custom css code (in autotoc.scss
) completely and go with all the standard Bootstrap stylings. Also the right floated vertical autotocs could be solved with Bootstraps float-end
or even responsive with float-md-end
css-classes. I think we can cut this pat-autotoc
pattern to a minimum here. What you think?
11df856
to
9d0ac8d
Compare
@petschki I agree with you; there are many redundant or overwritten parts of the styling and It is much better/clear to use standard Bootstrap styling instead. I have added two more commits to the PR related to this:
Some of the margin and padding values are not possible to replicate but I have tried to protect the same stylings for all the elements as much as possible. For example, the indentation of the table of content was 1, 2, and 3 rem, but bootstrap classes doesn't have 2 rem out of box so currently it is 1, 1.5, and 3 rem. Please let me know about your thoughts if we should either keep both commits or leave the styling of margins/paddings same by keeping it on the SCSS file. |
A general note: instead of merging master branch into the PR, try to rebase your changes onto master. This keeps the history cleaner (yeah I know, rebasing can be painful, but for encapsulated pattern changes it works nice) ... Second note: you can add commits with |
9d0ac8d
to
fd28f31
Compare
@petschki Thanks so much for the suggestions. I have applied your last suggested changes and rebased all them onto the master branch. Also, thanks for the tip about "git commit -n", I will use it next time :) |
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.
Thanks for the PR 🎉
But please bring back the <nav>
element.
Other than that, there is a lot of DOM manipulation going on here. I'd prefer having that in templates, e.g. the backend-rendered forms. But I think that's going beyond the scope of this PR and off the track of the original autotoc concept.
So fine for me with the <nav>
introduced back.
35b39b3
to
0d6c0d2
Compare
…nup custom styling.
0d6c0d2
to
ad0cf17
Compare
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.
Thanks! 🎉
This fixes first part of the plone/Products.CMFPlone#4106
This change updates the markup and behavior of the AutoTOC to make it more compatible with Bootstrap's tab component. Key changes: