Top 30 Albums Page #78
Conversation
|
Hi Tanzeela, can you change the PR to be more descriptive (ie Top 30 Albums Page). Additionally, please add a screenshot! Thanks :) You can use my PR for reference |
|
@binerys recommending her own PR as a reference LOL (In all seriousness though, it's a great PR.) Also, I noticed there were changes made to |
|
Updated the branch so that it's rebased with master per @nathunsmitty's suggestion. While rebasing with master, I fixed a routing conflict so the feature is accessible via localhost:3000/topalbums. Some additional comments:
|
| </div> | ||
|
|
||
|
|
||
| {/*albumsMonth.map(function(albumItem, index){ |
There was a problem hiding this comment.
I would remove this comment if it's unneccessary
| @@ -0,0 +1,142 @@ | |||
| // EventTab.jsx | |||
| // list of events in events tab of frontpage | |||
There was a problem hiding this comment.
Update comment to reflect feature
|
|
||
| var Album = React.createClass({ | ||
| render: function() { | ||
| console.log('within album', this.props); |
There was a problem hiding this comment.
Remove console log statement once feature is done
| <div className="albumText"> | ||
| <p className="albumName">{this.props.album}</p> | ||
| <p className="albumArtist">{this.props.artist}</p> | ||
| <a className="albumReview" href={this.props.albumreview}>Click for review</a> |
There was a problem hiding this comment.
What if no album review exists? I would utilize a conditional (perhaps a ternary so it's inline) to only display this link element if an album review exists.
| .albumItem{ | ||
| width: 32.666666666667%; | ||
| // height: 250px; | ||
| // padding-top: 24.66666666%; |
There was a problem hiding this comment.
Remove comments if they are unneccessary
| } | ||
| } | ||
| // .eventPage { | ||
| // margin: 10pt 0; |
There was a problem hiding this comment.
Remove these comments if they are unnecessary
|
5f6306b fixes the issue where album reviews should only appear if albumReview is specified. Some other things I noticed: But if a review is specified, the overlay looks like this: Can you fix it such that the white text is consistent for artist and album name are consistent throughout? |
|
Awesome work! Looks great with the changes |
Created grid layout for albums. Albums resize at different screen widths, but needs improvement. Tabs do not look like the mockup but are functional. Info in the panel version is as follows.
{ album: 'Enchant', artist: 'Emilie Autumn', imgurl: 'https://tytmb.files.wordpress.com/2011/10/enchant1.jpg', albumreview: '[tumblr-link-review]' }