3245 add breadcrumbs

[goratt12]

PR Checklist

• [ ] - Commit messages are descriptive, it will be used in our Release Notes

PR Type

• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Developer experience (improves developer workflows for contributing to the project)

Description

Add Breadcrumbs to the following pages:

• How-to
• Research
• Questions

By this method: Name of module > category > title of content (example: How-to > Moulds > Flowerpot Mould)

If the content doesn't have category, only two steps will show: Name of module > title of content (example: How-to > Flowerpot Mould)

Git Issues

Closes #3245

Screenshots/Videos

With category

Without category:

---

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

[github-actions[bot]]

Visit the preview URL for this PR (updated for commit 79c6ae2):

https://onearmy-next--pr3333-3245-add-breadcrumbs-whlg36p9.web.app

_{(expires Sat, 08 Jun 2024 10:53:32 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

[goratt12]

Currently the maximum length of the Breadcrumbs is 38 (if longer the middle steps are collapsed, and a tooltip is showed at hover) This parameter can be changed by the MAX_LENGTH const variable in src\pages\common\Breadcrumbs\Breadcrumbs.tsx

[davehakkens]

Nice @goratt12 Works very well! :) Got a few remarks: (im aware some are not flagged in the original design in the issue, sorry for that)

1. Could we change naming to singular? Like this:

• How-to
• Research
• Question

2. Could we have a bit more margin on top to avoid the logo going over the breadcrumbs on smaller screens

3. Can we align the breadcrumbs to the left with margin? So change this:

To this:

[goratt12]

Hi @benfurber ,

Thank you for your feedback!

Regarding the second point, I'd like to propose an alternative approach. What if we maintain a unified component structure and, instead of passing the steps as parameters, we provide the item (such as a research article, question, or how-to) along with a type/variant? This way, the steps could be determined internally within the component.

Looking forward to hearing your thoughts.

[benfurber]

Hi @benfurber ,

Thank you for your feedback!

Regarding the second point, I'd like to propose an alternative approach. What if we maintain a unified component structure and, instead of passing the steps as parameters, we provide the item (such as a research article, question, or how-to) along with a type/variant? This way, the steps could be determined internally within the component.

Looking forward to hearing your thoughts.

Sure! Sounds like this would DRY things up. Wonna give it a go and I'll have a look.

[davehakkens]

Are you able to wrap this one up @goratt12? Would be really nice to get this one merged

[goratt12]

Are you able to wrap this one up @goratt12? Would be really nice to get this one merged

Yeah I'm on it, Just wrapping things up with collaboration with @benfurber and will merged it :)

[benfurber]

@goratt12 Nice one. Lots of changes, thank you.

I've got a round of UI feedback first which it would be good to sort out before I code review again.

[Must fix]: The cursor doesn't change when hovering over a link https://github.com/ONEARMY/community-platform/assets/16688508/d4bfd5e6-77e5-4777-a7bd-10744ff9166c

[Must fix]: Some wierdly short titles to '...' on?

[Must fix]: How we link to Question categories has changed so the current. e.g. the link on https://onearmy-next--pr3333-3245-add-breadcrumbs-whlg36p9.web.app/questions/test-create to https://onearmy-next--pr3333-3245-add-breadcrumbs-whlg36p9.web.app/questions?category=testing+%F0%9F%8C%BD&sort=Newest should now go to https://onearmy-next--pr3333-3245-add-breadcrumbs-whlg36p9.web.app/questions?category=h1BTzeV9KWWFeP24ZtcT&sort=Newest

[sould fix]: Too much padding on mobile In case you haven't used it already, with 'sx' theme-ui has a convenient thing for that.

[could fix]: Arrows not aligned perfectly with text Should be easily fixable with the right flex setup.

And @davehakkens, can you weigh in on this one. I appreciate it was in the design work, but I'm not sure a seperate/new tooltip component is justified. Options (in my personal preference order):

1. Just use the existing one.
2. Update the existing one with some of the new style.
3. Update the existing one with a seperate style for breadcrumbs.

[davehakkens]

Thanks for adding the design comments @benfurber Looking at it live I actually noticed a few similair things as well (but was worried making this issue to big and feel bad not seeing this before the code started sorry @goratt12 )

I'd say ideally it looks more like this below because:

1. Categories are more imporant for navigation
2. Full title is shown below in content
3. Category length is in control of the admin, titles not
4. Tooltip can be skipped

[goratt12]

Hi @benfurber thanks for the feedback.

[Must fix]: The cursor doesn't change when hovering over a link

Done :)

[Must fix]: Some wierdly short titles to '...' on?

The '...' is set based on the total length of the entire Breadcrumbs component as set with the const BREADCRUMBS_ITEM_LABEL_MAX_LENGTH We can go towards the design @davehakkens suggested and shorten the title and not the category (makes more sense after reading the reasons) What do you think?

[Must fix]: How we link to Question categories has changed so the current.

Done :)

[sould fix]: Too much padding on mobile

Done, used the theme-ui sx prop with mobile support

[could fix]: Arrows not aligned perfectly with text

Done :)

[goratt12]

Thanks for adding the design comments @benfurber Looking at it live I actually noticed a few similair things as well (but was worried making this issue to big and feel bad not seeing this before the code started sorry @goratt12 )

I'd say ideally it looks more like this below because:

1. Categories are more imporant for navigation
2. Full title is shown below in content
3. Category length is in control of the admin, titles not
4. Tooltip can be skipped

I agree with the concept. With this design, there is no need for a tooltip at all, correct?

[davehakkens]

I agree with the concept. With this design, there is no need for a tooltip at all, correct?

Good to hear. Indeed no tooltip needed

[goratt12]

I made it look like @davehakkens suggested, and the title will only be shortened on mobile

[davehakkens]

Nice looks good to me. Thanks for making the edits. One thing I noticed that in the Research module clicking on the category (asdsd) brings me back to the research overview with all topics (not the filtered version with "asdsd" only

Seems to work on questions and how-to though

[benfurber]

@goratt12 Nice. The code is in a good place. Looks like we're close now.

[goratt12]

Nice looks good to me. Thanks for making the edits. One thing I noticed that in the Research module clicking on the category (asdsd) brings me back to the research overview with all topics (not the filtered version with "asdsd" only

Seems to work on questions and how-to though

It seems that the SortFilterHeader component doesn't read the URL category parameter. I think this should be solved in a different issue/PR. what do you think?

[mariojsnunes]

It seems that the SortFilterHeader component doesn't read the URL category parameter. I think this should be solved in a different issue/PR. what do you think?

SortFilterHeader will be removed soon as part of our ongoing search migration

[davehakkens]

Just one more little red cross ❌ 💪💪💪💪💪

[goratt12]

Just one more little red cross ❌ 💪💪💪💪💪

Fixed the testing issue, but still get a red cross. It looks like some random error in CircleCI

[mariojsnunes]

This PR needs a rebase/squash to remove extra commits as we use them for release notes. I've been using these steps to do it: https://stackoverflow.com/questions/30136558/how-to-squash-commits-which-have-merge-commit-in-between/69827502#69827502

And hopefully CircleCI doesn't randomly complain after that

[mariojsnunes]

We could have less coupling if each module would use <BreadcrumbsComponent> directly and generate it's own steps. So in practice Howto.tsx, ResearchArticle.tsx and QuestionPage.tsx would have their own generate steps function. Or create HowtoBreadcrumbs etc, components to encapsulate that.

Another advantage is that the <Breadcrumbs> component would no longer be needed and could rename BreadcrumbsComponent to simply Breadcrumbs.

[goratt12]

This PR needs a rebase/squash to remove extra commits as we use them for release notes. I've been using these steps to do it: https://stackoverflow.com/questions/30136558/how-to-squash-commits-which-have-merge-commit-in-between/69827502#69827502

And hopefully CircleCI doesn't randomly complain after that

No problem, I will squash the commits after the requested changes are resolved and no more commits are need :)

[goratt12]

We could have less coupling if each module would use <BreadcrumbsComponent> directly and generate it's own steps. So in practice Howto.tsx, ResearchArticle.tsx and QuestionPage.tsx would have their own generate steps function. Or create HowtoBreadcrumbs etc, components to encapsulate that.

Another advantage is that the <Breadcrumbs> component would no longer be needed and could rename BreadcrumbsComponent to simply Breadcrumbs.

This was the initial design (without the dedicated function) where each page calculated its steps for the Breadcrumbs component, @benfurber suggested creating a wrapper component for each page that would calculate the step but we decided to try using one wrapper component with the variant prop

2. For each component type, it would be great to get the changes on pages like src/pages/Question/QuestionPage.tsx down to a single line like <QuestionBreadcrumbs props, etc /> - e.g. a Wrapper component with all the logic for getting all the labelling data, etc. is hidden away.

@benfurber I would love to hear what you think, should we roll back to the original design or go with a wrapper per page component?

[benfurber]

Nice load of extra comments @mariojsnunes, thank you.

I'd say let's go with as is now. I think the common wrapper approach is fine and readible. So we added a new content type, it's easy to extend.

WE'RE SO CLOSE ON THIS NOW!

[mariojsnunes]

Sure, the wrapper is not a blocker for me

[mariojsnunes]

it's just missing that key, then ready to merge

[benfurber]

@goratt12 Still got a linting failure unfortunately.

[davehakkens]

stupid lint

[goratt12]

This PR needs a rebase/squash to remove extra commits as we use them for release notes. I've been using these steps to do it: https://stackoverflow.com/questions/30136558/how-to-squash-commits-which-have-merge-commit-in-between/69827502#69827502

And hopefully CircleCI doesn't randomly complain after that

I squashed the commits using this guide, should I open a new PR or keep the existing one?

[cypress[bot]]

Passing run #5536 ↗︎

0

80

1

0

0

Details:

Merge branch 'master' into 3245-add-breadcrumbs
Project: onearmy-community-platform	Commit: 79c6ae2837
Status: Passed	Duration: 04:18 💡
Started: May 8, 2024 3:41 PM	Ended: May 8, 2024 3:46 PM

Review all test suite changes for PR #3333 ↗︎