Skip to content
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

Inconsistency of breadcrumbs detail shown in about page of the website #6679

Closed
TenzDelek opened this issue Apr 29, 2024 · 9 comments · Fixed by #6710
Closed

Inconsistency of breadcrumbs detail shown in about page of the website #6679

TenzDelek opened this issue Apr 29, 2024 · 9 comments · Fixed by #6710
Labels

Comments

@TenzDelek
Copy link
Contributor

URL:

https://nodejs.org/en/about

Browser Name:

brave

Browser Version:

1.65.123

Operating System:

Windows 11

How to reproduce the issue:

  1. the behavior of the breadcrumb is not consistent across the sidebar in about page.
  2. the expected behavior should be like the part of the (Project governance) and the (branding of the nodejs) where the visual are expected.
image
  1. rest of the sub parts are only showing about nodejs as the title. the expected behavior is it should be like above mention
  2. current behavior problem screenshot:
image
  1. expected behavior:
image
@TenzDelek TenzDelek added the bug label Apr 29, 2024
@ovflowd
Copy link
Member

ovflowd commented Apr 29, 2024

Indeed, sounds like it is rendering the breadcrumbs inconsistently.

@TenzDelek
Copy link
Contributor Author

@ovflowd after going through the code base for a while i notice two thing.

  1. The path that doesnt have the dash(-) are rendering the breadcrumb as expected
image

Screenshot 2024-04-29 162744

  1. but the path which have the dash(-) in their path are not rendering correctly
image image

do you think that the issue lies in handling the dash or somewhere else?

@TenzDelek
Copy link
Contributor Author

found the issue regarding this problem,

the nodeWithCurrentPath for the paths which doesnt show the correct breadcrumbs are having undefined value.
Screenshot 2024-04-29 220449

whereas the path which are working fine have the values in nodeWithCurrentPath.
image

@ovflowd @AugustinMauroy what your guys thought on this? i tried debugging but was not able to come up with the solution. my assumption is regarding the mismatch of the pathname when we convert it to dashed.
image
the navigation tree has this values but the path that we are looking at has
image
which should be releases instead of previousReleases in the pathlist.

@TenzDelek
Copy link
Contributor Author

try: with a small logic of instead doing the camelcase i tried to remove the one side of the hypen
image
works with releases
before:
image
after:
image
but for the security reporting subpart ( the [0] pos is used) thats why it is not affecting.

also another issue is with the whole section of
image
the path is showing complety wrong
image
the current not path should be get involved instead of about i guess.
can you guys look at it and give your thoughts on how we can implement this changes. 😃

@TenzDelek
Copy link
Contributor Author

why get involved is set under about but about is set only one dynamic route
about ss:
image
get involved ss:
image

my doubt:

shouldnt the about route look same as the get involved like /about/about/branding instead of /about/branding

@mikeesto
Copy link
Member

mikeesto commented May 4, 2024

I'm not sure if this is related - just further down in the "Get Involved" section all of the breadcrumbs do not include the name of the page. For example: https://nodejs.org/en/about/get-involved/events

@tquocanvn
Copy link
Contributor

@TenzDelek @mikeesto

As I understand, there are 2 problems on the breadcrumbs handling:

  1. HOC withBreadcrums.tsx expected key name of side bar navigation in navigation.json to be matched with its last route path value, for example:
"releases": {
          "link": "/about/previous-releases",
          "label": "components.navigation.about.links.releases"
}

while it should be updated following:

"previousReleases": {
          "link": "/about/previous-releases",
          "label": "components.navigation.about.links.releases"
}

=> Update key name might be a proper solution for this case, but I'm not sure if changing key name might affect other places in the project or not, will need the help of @ovflowd to confirm about it.

  1. Section About Node.js and Get Involved are using same root path /about, therefore, when getting data for /about/get-involved/**, it will falls into data of About Node.js section while the correct one should be the other sectionGet Involved.

=> Update getBreadcrumbs function in withBreadcrumbs.tsx to check also child item route path will help to determine correct navigation data.

I've created a PR for this issue at #6710

@TenzDelek
Copy link
Contributor Author

@TenzDelek @mikeesto

As I understand, there are 2 problems on the breadcrumbs handling:

  1. HOC withBreadcrums.tsx expected key name of side bar navigation in navigation.json to be matched with its last route path value, for example:
"releases": {
          "link": "/about/previous-releases",
          "label": "components.navigation.about.links.releases"
}

while it should be updated following:

"previousReleases": {
          "link": "/about/previous-releases",
          "label": "components.navigation.about.links.releases"
}

=> Update key name might be a proper solution for this case, but I'm not sure if changing key name might affect other places in the project or not, will need the help of @ovflowd to confirm about it.

  1. Section About Node.js and Get Involved are using same root path /about, therefore, when getting data for /about/get-involved/**, it will falls into data of About Node.js section while the correct one should be the other sectionGet Involved.

=> Update getBreadcrumbs function in withBreadcrumbs.tsx to check also child item route path will help to determine correct navigation data.

I've created a PR for this issue at #6710

@tquocanvn your approach of passing the value seems correct, will close my pr!!

@abizek
Copy link
Contributor

abizek commented May 16, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants