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

feat: breadcrumbs #220

Merged
merged 3 commits into from
Feb 25, 2022
Merged

feat: breadcrumbs #220

merged 3 commits into from
Feb 25, 2022

Conversation

PatelN123
Copy link
Contributor

Adds the new breadcrumbs feature!

@PatelN123
Copy link
Contributor Author

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

@Josh-Cena
Copy link

Josh-Cena commented Feb 19, 2022

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

@PatelN123
Copy link
Contributor Author

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

@Josh-Cena
Copy link

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

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
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

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

slorber commented Feb 24, 2022

the current canary should now build fine without changing anything

facebook/docusaurus#6749

@PatelN123
Copy link
Contributor Author

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

@pranavrajs
Copy link
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