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 broken links in handbook #21686

Merged
merged 1 commit into from
Apr 21, 2020
Merged

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Apr 17, 2020

Description

This fixes a handful of links from the docs that were broken either in GitHub or in the Block Editor Handbook including the one mentioned in #15767.

Did a quick check via grep for relative links that don't match the criteria in the Using Links docs, confirmed which ones were actually broken, and updated them accordingly.

grep -REoh '\(/[^)]*\)' docs/**/*.md | \
	grep -Ev '/docs/.*\.md' | \
	grep -Ev '/packages/[^/]*/README.md' | \
	grep -Ev '/packages/components/src/.*/README.md'

Types of changes

Documentation fixes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@mkaz
Copy link
Member

mkaz commented Apr 17, 2020

Thanks for these contributions, it does fix a couple of links but I don't think we want to hardcode the links to the github.com repository, they should stay relative and on publish they get translated to Block Editor handbook links also.

Especially now since the handbook is being published off the release tag, and the links will point to the master branch, things will get out of sync or break when moved.

@ajlende
Copy link
Contributor Author

ajlende commented Apr 17, 2020

I don't think we want to hardcode the links to the github.com repository

@mkaz I agree. I suppose this is more of a stopgap to fix 404s until the code to generate the rest of the handbook is written.

There are already a lot of links hardcoded to GitHub that exist (40 results in 22 files in /docs that link to master), and many of the things that are linked to don't exist in the handbook right now (for example, all the components in /packages/block-editor/src/components).

I can roll back the ones that link to master in this PR if you'd like.

@ZebulanStanphill ZebulanStanphill added [Type] Bug An existing feature does not function as intended [Type] Developer Documentation Documentation for developers labels Apr 19, 2020
Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

I suppose a link to Github is better than broken outright. I'll approve and we can fix downstream.

I thought we had the deep linking into packages fixed but I can't find the corresponding ticket. @nosolosw was linking into packages resolved or still open?

@ajlende ajlende force-pushed the fix/15767-handbook-links branch from 7838ad9 to 05eb110 Compare April 21, 2020 13:57
@mkaz mkaz merged commit 6c01837 into WordPress:master Apr 21, 2020
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 21, 2020
@ajlende ajlende deleted the fix/15767-handbook-links branch April 21, 2020 15:30
@oandregal
Copy link
Member

I thought we had the deep linking into packages

I don't remember that we fixed this. However, I've also been a bit out of the loop in all regarding docs so I may have missed it.

Actually, now that this PR is merged but I don't see the changes in the handbook. Is that because we now pin the handbook to a branch? If that's the case, I wonder if we should cherry-pick commits the handbook branch from now on.

@mkaz
Copy link
Member

mkaz commented Apr 22, 2020

@nosolosw Yes, the handbook is pinned to wp/5.4 branch. Cherry-picking is probably a good idea, though the branch is locked and requires extra permissions to commit to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants