Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

Make 404 pages more useful #1558

Merged
merged 19 commits into from Oct 4, 2018
Merged

Make 404 pages more useful #1558

merged 19 commits into from Oct 4, 2018

Conversation

@fregante
Copy link
Collaborator

fregante commented Oct 3, 2018

dertieran added 4 commits Sep 15, 2018
The solution was overly specific, so we changed it to work for all 404s.
We ignore some paths that are never reachable (right now `tree` and `blob`).
For now it also doesn't change `blob` to `commits` anymore.
@fregante

This comment has been minimized.

Copy link
Collaborator

fregante commented on source/features/useful-not-found-page.js in 6541347 Sep 18, 2018

There’s a wrap function in utils.js

This comment has been minimized.

Copy link
Contributor Author

dertieran replied Sep 18, 2018

Hmm as far as I understand it wrap will wrap the target, but I want to wrap all children of the target.
Let me know if I misunderstand something here.

@dertieran

This comment has been minimized.

Copy link
Contributor Author

dertieran commented on source/features/useful-not-found-page.js in 6541347 Sep 18, 2018

Oh forgot to comment that, I wasn't sure if or what should be there.
So I went with the 404 theme and added a star wars quote 😅
Happy to change it to something more useful (or remove it)

fregante added 11 commits Oct 3, 2018
- Fetch the correct URL (it used to skip `tree/blob` entirely)
- Fetch from right to left to look nicer
- Reduce used elements and drop extra copy
- Use plain for loops instead of multiple functional loops and flatteners
It wouldn't return cached values
If not available on page
Because this way it matches the repo header
@fregante

This comment has been minimized.

Copy link
Collaborator Author

fregante commented Oct 3, 2018

screen shot 2018-10-03 at 17 39 35

@dertieran

This comment has been minimized.

Copy link
Contributor

dertieran commented Oct 4, 2018

Nice work, thanks for continuing on my work 👍
(Sorry for my PRs going stall, it's been a few busy weeks 😅)

// If the resource exists in the default branch, link it
async function addDefaultBranchLink(bar) {
const parts = getCleanPathname().split('/');
const [,,, branch] = parts;

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Oct 4, 2018

Owner

I don't think we should use destructuring when it's more than one lone ,.

This comment has been minimized.

source/features/useful-not-found-page.js Outdated Show resolved Hide resolved
@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Oct 4, 2018

This is incredible useful 🎉 I constantly end up on 404 pages and have to manually modify the URL.

@fregante

This comment has been minimized.

Copy link
Collaborator Author

fregante commented Oct 4, 2018

No worries @dertieran, thanks for starting this!

@sindresorhus sindresorhus changed the title Useful 404 pages Make 404 pages more useful Oct 4, 2018
@sindresorhus sindresorhus merged commit 0903681 into sindresorhus:master Oct 4, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sindresorhus

This comment has been minimized.

Copy link
Owner

sindresorhus commented Oct 4, 2018

Yay! 🔥

}
if (i === parts.length - 1) {
// The last part of the URL is a known 404
bar.append(' / ', getStrikeThrough(part));

This comment has been minimized.

Copy link
@yakov116

yakov116 Oct 4, 2018

Contributor

Small nitpick if the user is not found then the slash is appended.
Is that intended?

image

@yakov116

This comment has been minimized.

Copy link
Contributor

yakov116 commented Oct 4, 2018

Another one is https://github.com/sindresorhus/refined-github/issues/3515

Will ask i you want to see it on the default branch. It will include a link of https://github.com/sindresorhus/refined-github/issues/master Which puts in the search bar is:open is:issue author:master

@fregante fregante deleted the fregante:useful-404 branch Oct 5, 2018
@fregante

This comment has been minimized.

Copy link
Collaborator Author

fregante commented Oct 5, 2018

Excellent, I definitely didn't spend much time thinking about all the possible URLs. Can you open a joint issue for both? It should probably be limited to tree and blob URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.