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

fix(gatsby-theme-notes): Use basePath as home breadcrumb link #16564

Merged

Conversation

jakewies
Copy link
Contributor

@jakewies jakewies commented Aug 12, 2019

Description

Use the basePath option in gatsby-theme-notes to construct the link of the home breadcrumb icon.

Related Issues

Fixes #15494

@sidharthachatterjee
Copy link
Contributor

@jakewies Is this still necessary now that #16556 is in?

@jakewies
Copy link
Contributor Author

@sidharthachatterjee yes, it solves a different problem. However, if you read #15494 (comment) I brought up an issue that sticks out to me. If others agree with the issue, then I can re-work this PR.

@sidharthachatterjee
Copy link
Contributor

@jakewies Just saw the comment. The issue and your proposed solutions makes sense to me!

@jakewies
Copy link
Contributor Author

Ok great I'll make that change in this PR 👍

@sidharthachatterjee
Copy link
Contributor

Sounds good! Thanks @jakewies 👍

@jakewies jakewies requested review from a team as code owners August 14, 2019 01:42
@jakewies
Copy link
Contributor Author

jakewies commented Aug 14, 2019

@sidharthachatterjee so i definitely seem to have made a mistake here 😢

Sorry, still new at contributing to OSS. Let me see if I can explain:

I tried to update my PR branch locally with recent changes to the master branch, so here's what I've done:

# locally

git fetch upstream
git checkout master
git merge upstream/master

# check out local PR branch

git checkout themes/fix-breadcrumb-home-link
git rebase master

# make changes and commit

git commit -m "stuff"

# here's where things got fuzzy...

git push

I wasn't able to push and git informed me about a failed "non-fast-forward" (I should've taken a snapshot of the actual message, but I imagine this is pretty common for newcomers?).

I did some googling after the fact and came across this helpful Stackoverflow explanation.

So the main thing is:

Did I make a mistake by rebasing on master in the first place?

And how would I go about resolving this current predicament?

And finally, what should I have done to update this PR with recent changes from master so this never happens again 😅 Thanks!

@sidharthachatterjee
Copy link
Contributor

@jakewies No worries at all. It happens to the best of us! I think what happened is what you probably forgot to run git rebase --continue and committed instead?

I merged master in and fixed it. Typically I've seen that git merge master tends to be easier than git rebase master since a rebase goes commit by commit and a merge does not.

@jakewies
Copy link
Contributor Author

jakewies commented Aug 14, 2019

Ok awesome! So, just to recap:

The home breadcrumb is now linked to the basePath:

# home breadcrumb links to basePath

~ / example-dir

This structure also supports nested notes:

~ / example-dir / nested-dir

However, it seems that gatsby-node.js constructs the breadcrumbs in a way that prevents nested "breadcrumbs" from working. This doesn't mean that nested notes can't work in a project, it just means that nested breadcrumbs are broken.

# because of how gatsby-node constructs breadcrumbs, nested-dir
# will point to `basePath/nested-dir`, instead of `basePath/example-dir/nested-dir

~ / example-dir / nested-dir

I can make a fix for this as well if need be.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for now! Thank you so much @jakewies

Regarding nested notes, we don't currently them in the theme anyway so let's hold onto that

I think @ChristopherBiscardi is looking at cleaning this up shortly anyway

@sidharthachatterjee sidharthachatterjee changed the title fix: use basePath as breadcrumb-home-link fix(gatsby-theme-notes): Use basePath as home breadcrumb link Aug 14, 2019
@sidharthachatterjee sidharthachatterjee merged commit dcdbec9 into gatsbyjs:master Aug 14, 2019
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.

[gatsby-theme-notes] breadcrumb home icon is linking to “/“ instead of basePath.
3 participants