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

CB-270: Implemented the reviewal of artists #263

Merged
merged 11 commits into from Aug 7, 2019

Conversation

spellew
Copy link
Contributor

@spellew spellew commented Jun 16, 2019

No description provided.

@ferbncode
Copy link
Member

Let's start this as well. Could you fix the conflicts?

@spellew
Copy link
Contributor Author

spellew commented Jul 22, 2019

Merge conflict resolved.

@ferbncode
Copy link
Member

This is already good. If we could support coverart, it would be awesome, but it seems that the coverart API supports only release and release-groups at the moment.

However, do you think you can add a tab or list of existing reviews for an artist.
image
For example, for a release-group, a user can see the existing reviews as well.
image. You can come up with different ideas as to how to structure the page, and we can discuss and decide on one :)

@spellew
Copy link
Contributor Author

spellew commented Jul 27, 2019

@ferbncode So something like this?

Screenshot_2019-07-26 Fugees - CritiqueBrainz(2)
Screenshot_2019-07-26 Fugees - CritiqueBrainz(1)
Screenshot_2019-07-26 Fugees - CritiqueBrainz

@ferbncode
Copy link
Member

@spellew The third one looks good. We can limit to 5 reviews to show on artist page if there are many reviews. But the third one looks good, being outside the discography and very similar to release group pages.. What do you think?

@spellew
Copy link
Contributor Author

spellew commented Jul 27, 2019

@ferbncode I also like the third one, but I'm not sure if users having to scroll down to view the discography is a problem or not? If we limit it to five, we'd have arrows on the left or right, allowing users to view the next or previous reviews?

@ferbncode
Copy link
Member

@spellew I think there could be a link "See all reviews for"Artist"" which would list all the remaining reviews on the same page? If we find its not what users expect, we can come back again to fix it.

@spellew
Copy link
Contributor Author

spellew commented Jul 27, 2019

@ferbncode That sounds good to me.

@ferbncode
Copy link
Member

@spellew That looks good IMO :)

@spellew
Copy link
Contributor Author

spellew commented Jul 27, 2019

@spellew That looks good IMO :)

Changes have been pushed.

Copy link
Member

@ferbncode ferbncode left a comment

Choose a reason for hiding this comment

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

Slight nitpick, but other than that, everything looks good 👍

@@ -25,29 +25,42 @@ def entity(mbid):
# Note that some artists might not have a list of members because they are not a band
band_members = _get_band_members(artist)

reviews_limit = 5
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it configurable as a constant at the beginning of the file (and release_group_limit as well) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I get an example of what this would look like? I've attempted placing the constants at the beginning of the file, under the imports, but I keep getting UnboundLocalError: local variable referenced before assignment.

Copy link
Member

Choose a reason for hiding this comment

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

You can place then in __init__.py and then import the variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@ferbncode
Copy link
Member

@paramsingh When you get time, could you merge this and deploy to beta.critiquebrainz.org? Thanks in advance! :)

@paramsingh paramsingh merged commit dcbeca5 into metabrainz:master Aug 7, 2019
@paramsingh
Copy link
Collaborator

I've deployed it on beta, thanks for all the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants