nodejs / nodejs.org

The Node.js® Website
https://nodejs.org
MIT License
6.15k stars 6.22k forks source link

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

Closed TenzDelek closed 4 months ago

TenzDelek commented 5 months ago

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
  3. rest of the sub parts are only showing about nodejs as the title. the expected behavior is it should be like above mention

  4. current behavior problem screenshot:

    image
  5. expected behavior:

    image
ovflowd commented 5 months ago

Indeed, sounds like it is rendering the breadcrumbs inconsistently.

TenzDelek commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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. :smiley:

TenzDelek commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

@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 https://github.com/nodejs/nodejs.org/pull/6710

TenzDelek commented 5 months ago

@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 commented 4 months ago

I also found a few related issues with the breadcrumbs on the learn page. Particularly in these 4 articles

https://nodejs.org/en/learn/getting-started/ecmascript-2015-es6-and-beyond https://nodejs.org/en/learn/asynchronous-work/discover-javascript-timers https://nodejs.org/en/learn/asynchronous-work/event-loop-timers-and-nexttick https://nodejs.org/en/learn/manipulating-files/working-with-different-filesystems

I think we should consider these as well in the fix @tquocanvn