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-9736: Convert the artist search results page to React #678

Merged
merged 3 commits into from Jun 11, 2018

Conversation

mwiencek
Copy link
Member

@mwiencek mwiencek commented Jun 5, 2018

No description provided.

@mwiencek mwiencek requested a review from yvanzo June 5, 2018 19:45
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

IMHO, commits should make easy to track code/feature moving from Perl/TT to React/JSX. I would personally make only one commit for both removing the old TT template and adding the new JSX component. If that cannot be done within the same commit, I would add references in both commit messages.

) : (
<p>{l('No results found. Try refining your search query.')}</p>
)}
{$c.user && $c.user.is_location_editor ? (
Copy link
Contributor

@yvanzo yvanzo Jun 6, 2018

Choose a reason for hiding this comment

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

is_editing_enabled should be checked in place of is_location_editor.

!$c.user.is_editing_disabled should be checked in place of $c.user.is_location_editor.

$Subtype is an "unclear" type since 0.72.0, and it doesn't do anything
in these cases anyway.
@mwiencek
Copy link
Member Author

mwiencek commented Jun 6, 2018

I squashed the commit removing root/search/results-artist.tt into ff68ec6. I didn't remove root/search/lib/inline-results-artist.tt because I believe it's still being used by the taglookup pages.

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

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

root/search/lib/inline-results-artist.tt is referred from the template root/search/results-artist.tt you removed, thus it is fine for tracking code changes.

@mwiencek mwiencek merged commit dc8d3a6 into metabrainz:master Jun 11, 2018
@mwiencek mwiencek deleted the artist-results-react branch June 11, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants