Add autogenerated breadcrumbs component #2193

itsyme · 2023-03-01T09:15:14Z

What is the purpose of this pull request?

Overview of changes:
Resolves #2094.

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Add autogenerated breadcrumbs component

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

itsyme · 2023-03-13T17:02:17Z

I managed to do it looking at the site-nav DOM, however I am currently trying to iron out 2 bugs:

For the sitenav sections with only a dropdown (i.e. clicking on it only expands the dropdown), the breadcrumb is returning the first link instead of the dropdown. I can fix this if I can find a way to differentiate between only dropdown sections and dropdown with page sections
The breadcrumbs do not show for some pages and I cannot explain why. When I print the items array used to store the data for the breadcrumbs, it prints an empty array. I will have to look further into this to find out the root cause.

Would appreciate any feedback/suggestions!

ong6 · 2023-03-13T18:15:55Z

@itsyme Good work with the PR so far!

For the sitenav sections with only a dropdown (i.e. clicking on it only expands the dropdown), the breadcrumb is returning the first link instead of the dropdown. I can fix this if I can find a way to differentiate between only dropdown sections and dropdown with page sections

Maybe we can try to check this by looking if the dropdown has a link or not? if the dropdown has a link just link to it, else we just look at the first link.

The breadcrumbs do not show for some pages and I cannot explain why. When I print the items array used to store the data for the breadcrumbs, it prints an empty array. I will have to look further into this to find out the root cause.

Not sure what could be causing this issue but it might be the following?

We're using the document.querySelectorAll to find the site navigation. But what if the site nav is somehow not in the ul element with the class site-nav-list-root.
The site navigation structure is not consistent across all pages

itsyme · 2023-03-14T03:32:36Z

@itsyme Good work with the PR so far!

For the sitenav sections with only a dropdown (i.e. clicking on it only expands the dropdown), the breadcrumb is returning the first link instead of the dropdown. I can fix this if I can find a way to differentiate between only dropdown sections and dropdown with page sections

Maybe we can try to check this by looking if the dropdown has a link or not? if the dropdown has a link just link to it, else we just look at the first link.

The breadcrumbs do not show for some pages and I cannot explain why. When I print the items array used to store the data for the breadcrumbs, it prints an empty array. I will have to look further into this to find out the root cause.

Not sure what could be causing this issue but it might be the following?

We're using the document.querySelectorAll to find the site navigation. But what if the site nav is somehow not in the ul element with the class site-nav-list-root.

The site navigation structure is not consistent across all pages

Hi @ong6! Thanks for the suggestions! I managed to fix the second issue with some breadcrumbs not appearing. It seems that for some site-navs in different pages, there seems to be two uls with site-nav-list-root, even if they are in the same tier in the site-nav.

e.g. UG -> Authoring Contents -> Reusing Contents has 1 site-nav-list-roots but UG -> Authoring Contents -> Tweaking the page structure has 2.

Similarly, UG -> Authoring Contents -> Using Components -> Navigation has 2 but UG -> Authoring Contents -> Using Components -> The Rest has 1.

Do you think this could be a potential bug in the site-nav or if it intentionally made this way?

itsyme · 2023-03-15T00:45:32Z

The breadcrumbs works now! Just some small issues to iron out. I was wondering if we should visually distinguish between breadcrumbs that are clickable and those that are not. Currently, I do not distinguish between them as it made the breadcrumbs look messy.

Also, do we want to have a toggle for breadcrumbs in the frontmatter or just have the user add the breadcrumb component into their layout as the toggle?

jovyntls · 2023-03-15T02:10:40Z

I was wondering if we should visually distinguish between breadcrumbs that are clickable and those that are not.

Ideally yes, but I agree that it may look messy. I can't think of a cleaner way to go about this besides having the usual cursor-hover behaviour on links.

Also, do we want to have a toggle for breadcrumbs in the frontmatter or just have the user add the breadcrumb component into their layout as the toggle?

I think both are viable! Adding the breadcrumbs component into their layout that could offer users more flexibility and allow them to include it in the default layout. On the other hand, 90% of the time the breadcrumbs are probably in the same place, so having it as a frontmatter toggle could be more convenient.

I'm leaning towards having it as a component to include in layouts since this aligns more with how we support other features like pageNavs, etc.

itsyme · 2023-03-15T03:20:00Z

I was wondering if we should visually distinguish between breadcrumbs that are clickable and those that are not.

Ideally yes, but I agree that it may look messy. I can't think of a cleaner way to go about this besides having the usual cursor-hover behaviour on links.

Also, do we want to have a toggle for breadcrumbs in the frontmatter or just have the user add the breadcrumb component into their layout as the toggle?

I think both are viable! Adding the breadcrumbs component into their layout that could offer users more flexibility and allow them to include it in the default layout. On the other hand, 90% of the time the breadcrumbs are probably in the same place, so having it as a frontmatter toggle could be more convenient.

I'm leaning towards having it as a component to include in layouts since this aligns more with how we support other features like pageNavs, etc.

Thanks for the feedback! I am doing the documentation for Breadcrumbs now and am wondering if I should put it under "Navigation" or "Others". I am leaning towards "Others" as the "Navigation" section is already very long and breadcrumbs is more a peripheral component rather than a core navigation component. What do you think?

lhw-1 · 2023-03-15T05:16:00Z

I am doing the documentation for Breadcrumbs now and am wondering if I should put it under "Navigation" or "Others". I am leaning towards "Others" as the "Navigation" section is already very long and breadcrumbs is more a peripheral component rather than a core navigation component. What do you think?

Hi @itsyme, thanks for the PR! I personally think that since breadcrumbs fundamentally still has to do with navigation between pages, I think putting it under the "Navigation" section still makes more sense than "Others". (As a side note, I feel like we can afford to also rename the "Others" into "Questions and Quizzes" since there's only those components described in the "Others" section)

docs/userGuide/syntax/breadcrumbs.md

damithc · 2023-03-15T06:20:31Z

@itsyme For new features, also good to try them out on CS2103/T website as that's the heaviest MarkBind site we have now.
e.g., https://github.com/nus-cs2103-AY2223S2/website
Check how the new feature affects various pages, including the the textbook (https://nus-cs2103-ay2223s2.github.io/website/se-book-adapted/index.html), printer-friendly pages in particular.

ong6 · 2023-03-15T12:45:33Z

The breadcrumbs works now! Just some small issues to iron out. I was wondering if we should visually distinguish between breadcrumbs that are clickable and those that are not. Currently, I do not distinguish between them as it made the breadcrumbs look messy.

Great work so far! What do you think about leaving those unclickable links out of the breadcrumbs completely. (don't show them all together.) My reasoning for this is because breadcrumbs are used mainly for navigation and if there is no link, perhaps there is no need to show them since they can't navigate to it anyways.

Also, do we want to have a toggle for breadcrumbs in the frontmatter or just have the user add the breadcrumb component into their layout as the toggle?

I agree with Jovyn on this point. Leaving it in layouts seems right.

tlylt · 2023-03-15T13:02:48Z

Can you also share what it looks like for longer-than-usual titles (hence a longer breadcrumb), to see if it handles that well?

itsyme · 2023-03-15T15:51:25Z

Great work so far! What do you think about leaving those unclickable links out of the breadcrumbs completely. (don't show them all together.) My reasoning for this is because breadcrumbs are used mainly for navigation and if there is no link, perhaps there is no need to show them since they can't navigate to it anyways.

I think that leaving the unclickable links out might reduce the value that the breadcrumbs bring to the user.

I find the unclickable links being displayed helps the user to track which section they currently are more easily, even if the links do not direct them to a page on click.

Also, much of the sitenav dropdowns (that I have seen so far) do not contain a page. This could lead to our breadcrumb displaying only the current page majority of the time. What do you think?

jovyntls

Thanks @itsyme , the component is working well!

Is it possible to add tests for generating the items array from the siteNav, since the current tests stub the items array?

I'm also wondering if we should add this to the default template 🤔 What do you think?

docs/userGuide/syntax/breadcrumbs.md

jovyntls · 2023-03-28T13:35:35Z

packages/vue-components/src/Breadcrumb.vue

@@ -0,0 +1,102 @@
+<template>
+  <div>
+    <nav aria-label="breadcrumb">


This is a really nice & thoughtful accessibility consideration! Same for the aria-current below 👍

packages/vue-components/src/Breadcrumb.vue

jovyntls · 2023-03-28T13:52:57Z

packages/vue-components/src/Breadcrumb.vue

+
+    // Identify the first site-nav-list-root
+    // In the ideal case, there is only one site-nav-list-root
+    // however this is not the case for all pages


:O just checking, would the other site-nav-list-root be a duplicate? Or is it possible to have multiple site-nav-list-root with different contents?

From my experience, the other site-nav-list-root is a subset of the current sitenav. I feel like this is unexpected behaviour as ideally there should only be one site-nav-list-root

I have found out why the bug is happening! I realised that the navigation page has an example sitenav in the page, resulting in the weird behaviour.

packages/vue-components/src/Breadcrumb.vue

jovyntls · 2023-03-28T14:43:27Z

Not a big issue, but I'm getting a weird jerky change of the cursor when moving to an .active class:

I'm fine with removing the class entirely since having the cursor as a caret when hovering over text seems reasonable to me, given that that's the standard cursor behaviour.

Screen.Recording.2023-03-28.at.9.43.29.PM.mov

itsyme · 2023-03-29T01:49:29Z

Thanks @itsyme , the component is working well!

Is it possible to add tests for generating the items array from the siteNav, since the current tests stub the items array?

I'm also wondering if we should add this to the default template 🤔 What do you think?

Thanks for the review @jovyntls! I was having some issues with the tests with siteNav and might need some time to work it to get them to work. I will try to get it done ASAP! As for the default template, I think it would be a great addition! I will add it in the next commit!

itsyme · 2023-03-30T06:07:03Z

Thanks @itsyme , the component is working well!

Is it possible to add tests for generating the items array from the siteNav, since the current tests stub the items array?

I'm also wondering if we should add this to the default template 🤔 What do you think?

HI @jovyntls! I have made the relevant changes including generating the breadcrumbs from the sitenav component. However, because it requires a short timeout, I have skipped the tests because of issue #2238 just to be sure, but I have tested them and they work as intended.

jovyntls · 2023-03-30T08:32:15Z

I see, why does it require a setTimeout? Is it to wait for the breadcrumbs to be generated after mounting?

itsyme · 2023-03-30T09:06:23Z

I see, why does it require a setTimeout? Is it to wait for the breadcrumbs to be generated after mounting?

Yup! There needs to be a very short delay for the breadcrumbs to generate the data from the sitenav, else the snapshot will return an empty breadcrumb!

jovyntls · 2023-03-30T15:08:20Z

Would the nextTick() method fix this? 🤔 Something like await nextTick() to wait for the component to mount and run the generating breadcrumbs method.

For docs, ctrl-F 'nextTick' here: https://test-utils.vuejs.org/guide/advanced/async-suspense.html#a-simple-example-updating-with-trigger

Unfortunately this won't work for the other tests in #2238 since those components implicitly use setTimeouts 😢

itsyme · 2023-03-31T01:13:12Z

Would the nextTick() method fix this? 🤔 Something like await nextTick() to wait for the component to mount and run the generating breadcrumbs method.

For docs, ctrl-F 'nextTick' here: https://test-utils.vuejs.org/guide/advanced/async-suspense.html#a-simple-example-updating-with-trigger

Unfortunately this won't work for the other tests in #2238 since those components implicitly use setTimeouts 😢

Thanks for the suggestion! It worked well and I have implemented it in the latest commit and unskipped the tests!

jovyntls

LGTM! This would be a nice feature addition 🎉 Thanks @itsyme !

itsyme added 8 commits February 12, 2023 08:49

Add Breadcrumb component

a49697a

Merge branch 'master' into add-breadcrumbs

9b2053d

Merge branch 'master' into add-breadcrumbs

8446b20

Update Breadcrumb component to generate from url

9e52b2c

Merge branch 'master' into add-breadcrumbs

26efe57

Merge branch 'master' into add-breadcrumbs

470d8c4

Merge branch 'master' into add-breadcrumbs

f81ad86

Change approach to use site-nav

62f6a36

itsyme added 4 commits March 14, 2023 11:33

Fix missing breadcrumb issue

e130ef4

Fix breadcrumbs returning first link instead of folder

3736018

Change style for non-link breadcrumbs

d1a11c8

Merge branch 'master' into add-breadcrumbs

52e0ee0

itsyme marked this pull request as ready for review March 15, 2023 01:24

Remove showBreadcrumb

eaaed3d

itsyme added 2 commits March 15, 2023 12:18

Add documentation for Breadcrumbs

58af2cc

Add bold

40f2420

lhw-1 reviewed Mar 15, 2023

View reviewed changes

docs/userGuide/syntax/breadcrumbs.md Show resolved Hide resolved

itsyme added 2 commits March 15, 2023 17:22

Relocate to Navigation and add example

1577de8

Fix nits in docs

bcda8c0

itsyme added 5 commits March 27, 2023 21:33

Add snapshot test for breadcrumbs

838cf04

Undo breadcrumb in test site for merge

6dfcfc2

Merge branch 'master' into add-breadcrumbs

6c5b9ba

Add breadcrumb to test site

3d88001

Fix typo in test

d388747

itsyme requested a review from jovyntls March 28, 2023 06:50

itsyme added 2 commits March 28, 2023 14:50

Merge branch 'master' into add-breadcrumbs

34ba094

Merge branch 'master' into add-breadcrumbs

301881d

jovyntls suggested changes Mar 28, 2023

View reviewed changes

itsyme added 3 commits March 29, 2023 09:21

Make improvements

dba91ad

Make improvements

aa0a123

Merge branch 'master' into add-breadcrumbs

4c6c68c

itsyme added 3 commits March 29, 2023 09:54

Add breadcrumb to default template

95d1c28

Update tests

45bae02

Add tests that query from sitenav

c673b2f

Merge branch 'master' into add-breadcrumbs

d566ac3

Use nextTick for tests

e91d38e

jovyntls approved these changes Mar 31, 2023

View reviewed changes

itsyme added this to the v5.0.0 milestone Mar 31, 2023

itsyme and others added 2 commits April 1, 2023 22:01

Merge branch 'master' into add-breadcrumbs

52c6edf

Update tests

e6b98df

itsyme changed the title ~~Add breadcrumbs~~ Add autogenerated breadcrumbs component Apr 1, 2023

itsyme merged commit aa1fdbb into MarkBind:master Apr 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add autogenerated breadcrumbs component #2193

Add autogenerated breadcrumbs component #2193

itsyme commented Mar 1, 2023 •

edited

Loading

itsyme commented Mar 13, 2023

ong6 commented Mar 13, 2023

itsyme commented Mar 14, 2023 •

edited

Loading

itsyme commented Mar 15, 2023

jovyntls commented Mar 15, 2023

itsyme commented Mar 15, 2023

lhw-1 commented Mar 15, 2023

damithc commented Mar 15, 2023

ong6 commented Mar 15, 2023 •

edited

Loading

tlylt commented Mar 15, 2023

itsyme commented Mar 15, 2023

jovyntls left a comment •

edited

Loading

jovyntls Mar 28, 2023

jovyntls Mar 28, 2023

itsyme Mar 29, 2023

itsyme Mar 29, 2023

jovyntls commented Mar 28, 2023

itsyme commented Mar 29, 2023

itsyme commented Mar 30, 2023 •

edited

Loading

jovyntls commented Mar 30, 2023

itsyme commented Mar 30, 2023

jovyntls commented Mar 30, 2023 •

edited

Loading

itsyme commented Mar 31, 2023

jovyntls left a comment

Add autogenerated breadcrumbs component #2193

Add autogenerated breadcrumbs component #2193

Conversation

itsyme commented Mar 1, 2023 • edited Loading

itsyme commented Mar 13, 2023

ong6 commented Mar 13, 2023

itsyme commented Mar 14, 2023 • edited Loading

itsyme commented Mar 15, 2023

jovyntls commented Mar 15, 2023

itsyme commented Mar 15, 2023

lhw-1 commented Mar 15, 2023

damithc commented Mar 15, 2023

ong6 commented Mar 15, 2023 • edited Loading

tlylt commented Mar 15, 2023

itsyme commented Mar 15, 2023

jovyntls left a comment • edited Loading

Choose a reason for hiding this comment

jovyntls Mar 28, 2023

Choose a reason for hiding this comment

jovyntls Mar 28, 2023

Choose a reason for hiding this comment

itsyme Mar 29, 2023

Choose a reason for hiding this comment

itsyme Mar 29, 2023

Choose a reason for hiding this comment

jovyntls commented Mar 28, 2023

itsyme commented Mar 29, 2023

itsyme commented Mar 30, 2023 • edited Loading

jovyntls commented Mar 30, 2023

itsyme commented Mar 30, 2023

jovyntls commented Mar 30, 2023 • edited Loading

itsyme commented Mar 31, 2023

jovyntls left a comment

Choose a reason for hiding this comment

itsyme commented Mar 1, 2023 •

edited

Loading

itsyme commented Mar 14, 2023 •

edited

Loading

ong6 commented Mar 15, 2023 •

edited

Loading

jovyntls left a comment •

edited

Loading

itsyme commented Mar 30, 2023 •

edited

Loading

jovyntls commented Mar 30, 2023 •

edited

Loading