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

chore(feat): localize 404 page (fix #1987) #2004

Closed
wants to merge 12 commits into from
Closed

chore(feat): localize 404 page (fix #1987) #2004

wants to merge 12 commits into from

Conversation

sybers
Copy link

@sybers sybers commented Nov 6, 2019

Summary

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

This PR adds localized messages to the 404 page, matching the language of the URL.
It is still missing some Chinese translations for the default messages which I am not able to provide.

Comment on lines 38 to 44
notFoundLinkText: 'Take me home.',
notFoundMessages: [
`There's nothing here.`,
`How did we get here?`,
`That's a Four-Oh-Four.`,
`Looks like we've got some broken links.`
]
Copy link
Author

Choose a reason for hiding this comment

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

Needs translation.

Comment on lines 46 to 52
"notFoundLinkText": "Take me home.",
"notFoundMessages": [
"There's nothing here.",
"How did we get here?",
"That's a Four-Oh-Four.",
"Looks like we've got some broken links."
]
Copy link
Author

Choose a reason for hiding this comment

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

Needs translation.

Comment on lines 66 to 72
notFoundLinkText: 'Take me home.',
notFoundMessages: [
`There's nothing here.`,
`How did we get here?`,
`That's a Four-Oh-Four.`,
`Looks like we've got some broken links.`
],
Copy link
Author

Choose a reason for hiding this comment

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

Needs translation.

@@ -54,7 +54,7 @@ module.exports = siteData => {
for (const path in locales) {
if (path === '/') {
defaultLang = locales[path]
} else if (this.$page.path.indexOf(path) === 0) {
} else if (this.$page.path.indexOf(path) === 0 || (!this.$page.path && this.$route.path.indexOf(path) === 0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are you trying to solve with this condition? When would this.$page.path not be defined?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dzetah ?

@@ -42,6 +42,13 @@ module.exports = ctx => ({
selectText: 'Languages',
ariaLabel: 'Select language',
editLinkText: 'Edit this page on GitHub',
notFoundLinkText: 'Take me home.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to set this for the / locale as these are the default values right?

// TODO: comment me
notFoundLinkText: 'Take me home.',
// TODO: comment me
notFoundMessages: [/* ... */],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here (to translate)

@@ -54,6 +54,8 @@ module.exports = {
label: 'English',
ariaLabel: 'Languages'
editLinkText: 'Edit this page on GitHub',
notFoundLinkText: 'Take me home.',
notFoundMessages: [/* ... */],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here (to translate)


// custom text for 404 page link. Defaults to "Take me home."
notFoundLinkText: 'Take me home.',
notFoundMessages: [/* ... */]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is important to write an example with the default values here, to let the user know what the default values are.

@@ -79,6 +83,8 @@ module.exports = {
selectText: '选择语言',
label: '简体中文',
editLinkText: '在 GitHub 上编辑此页',
notFoundLinkText: 'Take me home.',
notFoundMessages: [/* ... */],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here

Copy link
Collaborator

@kefranabg kefranabg left a comment

Choose a reason for hiding this comment

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

That is great work @dzetah! Just small things to fix and one question to answer to!

@kefranabg
Copy link
Collaborator

@newsbielt703 or @meteorlxy could you help with the zh translation?

@kefranabg
Copy link
Collaborator

kefranabg commented Nov 10, 2019

@dzetah Sorry I did not that some sentences were using apostrophes 😅 In this case you can wrap these sentences with " instead of using \' in the sentences.

@sybers
Copy link
Author

sybers commented Dec 6, 2019

Hello, any news on this ? I'd like to see this merged soon :)

I think it only lacks the Chinese translation (docs and defaults).

@kefranabg
Copy link
Collaborator

@dzetah yes, we just need one more approve and zh translation (@meteorlxy ?) BTW could you answer this question in my review ?

What are you trying to solve with this condition? When would this.$page.path not be defined?

@sybers
Copy link
Author

sybers commented Dec 6, 2019

@kefranabg I answered this question before but it looks like you can't see my answer so i'll post it here below too :)

When the page wasn't found (before it redirects to the 404 page, this.$page.path is set to ''. So in this case the idea was to match the locale directly from the URL instead of the page path.

@meteorlxy
Copy link
Member

meteorlxy commented Dec 10, 2019

I've finished the Chinese translation.

Also resolved conflicts, linted the code, and made some tweaks.

@meteorlxy
Copy link
Member

Currently, it will show the default messages first, and change to the locale messages.

It's not so perfect 🤔

https://deploy-preview-2004--vuepress.netlify.com/zh/guide/aaa

demo-locale

@kefranabg
Copy link
Collaborator

@dzetah Could you fix this? We're not supposed to have this blink effect

@meteorlxy
Copy link
Member

meteorlxy commented Dec 10, 2019

Although it's not a critical problem, I think it would be a little complicated to fix that.

Maybe vuepress-plugin-dehydrate will help ---- to dehydrate the 404.html page, because 404 pages are not necessary to be pre-rendered.

@sybers
Copy link
Author

sybers commented Dec 10, 2019

@kefranabg Obviously this is not an expected behaviour, I'll try to take a look at this later this week...

Thanks @meteorlxy for your improvements :)

@haoranpb haoranpb mentioned this pull request Apr 1, 2020
1 task
@sybers
Copy link
Author

sybers commented Apr 1, 2020

@meteorlxy I'm not sure vuepress-plugin-dehydrate will help us in this case because the 404 component is replacing the page at the given location. Maybe I'm misunderstanding the issue here 🤔

@sybers
Copy link
Author

sybers commented Apr 1, 2020

@meteorlxy @kefranabg I think we could just use the <client-only> component for the 404 page content, so it will be ignored during SSR, what do you think ?

@sybers
Copy link
Author

sybers commented Jul 7, 2021

@kefranabg I opened this PR a year ago and I just stumbled upon it again today 😅

Is this still something that can be merged ?

@meteorlxy
Copy link
Member

@dzetah See #2744.

BTW, it's already supported in the default theme of VuePress v2

@sybers
Copy link
Author

sybers commented Jul 9, 2021

Hi @meteorlxy thanks for your answer, I'm going to use VuePress 2 from now on then :)

I guess we can decline this PR.

@sybers sybers closed this Jul 9, 2021
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.

3 participants