-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add support for ToC sublists #520
Conversation
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/story-ramp/toc-sublists/#/en/00000000-0000-0000-0000-000000000000 |
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.
Looks great and works from an accessibility perspective as well!
The table of contents config doesn't seem to be respected on mobile. I think these changes also need to be in the mobile-menu
component?
Reviewed 4 of 7 files at r1, all commit messages.
Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @yileifeng)
src/components/story/chapter-menu.vue
line 156 at r1 (raw file):
}" > <div class="flex">
Feel free to shut this down if I'm missing something, but I think we could simplify this component by breaking out the code inside this flex
div into its own reusable component.
Currently, the plugin
and not plugin
blocks are repeated three times: once when there is no ToC config, and twice more when a ToC config exists (once for the main chapter and once for sub-chapters). By moving this repeating block into its own component, we could significantly reduce redundancy and make the code more manageable.
(definitely did not ask ChatGPT to clarify my words ^)
src/components/story/chapter-menu.vue
line 223 at r1 (raw file):
</router-link> <button
This button will need an aria-label
src/components/story/horizontal-menu.vue
line 50 at r1 (raw file):
}" > <div class="flex">
Same comment as the last file for this, since we're repeating it 3 times here as well we can make this file a lot cleaner by reusing the component.
src/components/story/horizontal-menu.vue
line 87 at r1 (raw file):
</router-link> <button class="mr-2" v-if="item.sublist && item.sublist.length" @click="toggleSublist(idx)">
Same as above, aria-label
cdc26f9
to
718fe07
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.
Good catch, mobile menu should have the changes integrated as well now
Reviewed 3 of 7 files at r1, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/story/chapter-menu.vue
line 156 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Feel free to shut this down if I'm missing something, but I think we could simplify this component by breaking out the code inside this
flex
div into its own reusable component.Currently, the
plugin
andnot plugin
blocks are repeated three times: once when there is no ToC config, and twice more when a ToC config exists (once for the main chapter and once for sub-chapters). By moving this repeating block into its own component, we could significantly reduce redundancy and make the code more manageable.(definitely did not ask ChatGPT to clarify my words ^)
Good point, I added a helper component and refactored all three menu components to utilize this. Code is a lot cleaner, but have to believe that if this is possible then we should also explore refactoring all three components into one.
src/components/story/chapter-menu.vue
line 223 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
This button will need an
aria-label
Donethanks
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.
Changes look good!
This is not very important, but I think the padding/margin for vertical ToC sub-lists can be reduced slightly to allow for more text to appear (removed m-4
from the first sub-list vs left as-is for the second one):
Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
src/components/story/chapter-menu.vue
line 156 at r1 (raw file):
Previously, yileifeng (Yi Lei Feng) wrote…
Good point, I added a helper component and refactored all three menu components to utilize this. Code is a lot cleaner, but have to believe that if this is possible then we should also explore refactoring all three components into one.
718fe07
to
81a2ae6
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.
Good idea, kept the margin only for vertical TOC parent items
Reviewable status: 3 of 11 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)
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.
Should the menus close when the user clicks elsewhere (or moves keyboard focus out of the menu)? Right now you can open all of the sublists and they all stay open while you do other things.
Reviewable status: 3 of 11 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA and @yileifeng)
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 should be fixed now, menus close after mouse clicking elsewhere or when focus is off the ToC item for keyboard users. Let me know if current behavior is accurate or there are more cases I missed.
Reviewable status: 2 of 11 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)
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.
Also given that there can only be one submenu open for horizontal ToC, I refactored the component to only track the index of the dropdown that is toggled instead of tracking all items keys.
Reviewable status: 2 of 11 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)
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.
Looks good to me 👍
Reviewed 3 of 7 files at r1, 8 of 8 files at r5.
Reviewable status: 7 of 11 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA and @yileifeng)
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.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)
Related Item(s)
Closes #492
Changes
Testing
For horizontal ToC: https://ramp4-pcar4.github.io/story-ramp/toc-sublists/#/en/00000000-0000-0000-0000-000000000000
For vertical ToC: https://ramp4-pcar4.github.io/story-ramp/toc-sublists/#/fr/00000000-0000-0000-0000-000000000000
This change is