Skip to content

feat: breadcrumbs#220

Merged
pranavrajs merged 3 commits intochatwoot:mainfrom
PatelN123:Add-breadcrumbs
Feb 25, 2022
Merged

feat: breadcrumbs#220
pranavrajs merged 3 commits intochatwoot:mainfrom
PatelN123:Add-breadcrumbs

Conversation

@PatelN123
Copy link
Copy Markdown
Contributor

Adds the new breadcrumbs feature!

@PatelN123
Copy link
Copy Markdown
Contributor Author

I think this is failing due to the sidebar configuration.
I will check back with this later.

@Josh-Cena
Copy link
Copy Markdown

Josh-Cena commented Feb 19, 2022

I'm noted. I'll inspect this—could be a Docusaurus regression

@PatelN123
Copy link
Copy Markdown
Contributor Author

Thanks, I knew you'd be able to figure it out.

@Josh-Cena
Copy link
Copy Markdown

Josh-Cena commented Feb 19, 2022

The issue is exactly related to the breadcrumbs. The breadcrumbs, by default, contains a link to the "home page", which is https://www.chatwoot.com/docs – which is non-existent. Docusaurus doesn't know that this link re-directs and simply reports that it doesn't exist.

@slorber Do you think we should simply apply data-noBrokenLinkCheck for the home page link? I don't believe there's any site whose home page is a 404, so if it doesn't exist, it must be redirected.

@PatelN123 I don't think you should upgrade the site to canary version just for a certain feature. Just wait until the stable release is out (which should be next week—we release about every month).

@slorber
Copy link
Copy Markdown

slorber commented Feb 23, 2022

That's annoying 😅

A quickfix would be to use slug: / to the self-hosted.md page so that your site has a homepage

This means https://www.chatwoot.com/docs/self-hosted would become https://www.chatwoot.com/docs/ (and you should remove the server redirect): not sure it's something you want.

@slorber Do you think we should simply apply data-noBrokenLinkCheck for the home page link? I don't believe there's any site whose home page is a 404, so if it doesn't exist, it must be redirected.

The redirect is server-side, so if we navigate with <Link to="/"> this will navigate client-side and display a 404 anyway 😓

It'd say we can first apply data-noBrokenLinkCheck to permit build but we should probably warn the user that the breadcrumb won't work as intended without a docusaurus homepage, and they eventually need to swizzle it?

Maybe we could find a heuristic so that there's always a site homepage to redirect to? (docs or blog for example)

Maybe we should hide the home item if we detect the site has no homepage?

Let's continue this discussion in facebook/docusaurus#6517 (comment)

@PatelN123 I don't think you should upgrade the site to canary version just for a certain feature. Just wait until the stable release is out (which should be next week—we release about every month).

On the contrary I think it's good for us and actually, help us see these kinds of bugs earlier :D

@pranavrajs
Copy link
Copy Markdown
Member

pranavrajs commented Feb 23, 2022

@slorber Thanks for taking a look at the PR.

This means https://www.chatwoot.com/docs/self-hosted would become https://www.chatwoot.com/docs/ (and you should remove the server redirect): not sure it's something you want.

We already had /help-center on our marketing website, didn't want to duplicate it. I guess we could move it to Docusaurus instead of keeping it there. It would be easier that way to get this upgrade completed.

Glad that we found the bug early. :)

@PatelN123 Thanks for the upgrade and the proactive communication on this.

@slorber
Copy link
Copy Markdown

slorber commented Feb 23, 2022

👍

imho you can keep /help-center in your gatsby site, you just need a real page at /docs

@slorber
Copy link
Copy Markdown

slorber commented Feb 24, 2022

the current canary should now build fine without changing anything

facebook/docusaurus#6749

@PatelN123
Copy link
Copy Markdown
Contributor Author

Thanks @slorber, I'll give it a go!

@pranavrajs
Copy link
Copy Markdown
Member

@PatelN123 @slorber Thanks for the work, merging this as the update is working as expected.

@pranavrajs pranavrajs merged commit 937d1c6 into chatwoot:main Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants