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

MBS-9607: Rewrite MusicBrainz not found pages in React/JSX #602

Merged
merged 2 commits into from
Feb 16, 2018

Conversation

spellew
Copy link
Contributor

@spellew spellew commented Jan 17, 2018

Replaced all the not_found.tt files with equivalent templates written in React/JSX. Accepted on Google Code-In by yvanzo.

@yvanzo
Copy link
Contributor

yvanzo commented Jan 18, 2018

Depends on PR #599.

@mwiencek
Copy link
Member

I think we should create a reusable NotFound component to reduce some of the duplication here. Here's an example of what that would look like:

const EditNotFound = () => (
  <NotFound
    title={l('Edit Not Found')}
    message={l('Sorry, we could not find an edit with that edit ID. You may wish to try and perform an {search_url|edit search} instead.', {__react: true, search_url: '/search/edits'})}
  />
);

const TagNotFound = () => (
<NotFound title={l('Tag Not Used')}>
<p>{l('No MusicBrainz entities have yet been tagged with "' + $c.stash.tag + '".')}</p>
<p>{l('If you wish to use this tag, please {search_url|search} for the entity first and apply the tag using the sidebar.',
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the strings unless necessary. I saw you changed {url|search} to {search_url|search} and had to go through all the rest to make sure there weren't other changes. These changes require translators to retranslate things for no reason.

(No need to push fixes, I'm taking care of it.)

@mwiencek
Copy link
Member

Fixed it so that the pages actually still 404 in 9902927.

@mwiencek mwiencek merged commit 4db1ffd into metabrainz:master Feb 16, 2018
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