Featured artists module rebase #2032
Conversation
Co-authored-by: Christina Thompson <christina@artsymail.com> # Conflicts: # data/complete.queryMap.json
Co-authored-by: Christina Thompson <christina@artsymail.com>
Co-authored-by: Christina Thompson <christina@artsymail.com>
Co-authored-by: Christina Thompson <christina@artsymail.com>
ashleyjelks
force-pushed
the
featured-artists-module-rebase
branch
from
c9f761c to
2c290f2
Compare
| <Box width="100%" key={index} pb={20}> | ||
| <TouchableWithoutFeedback onPress={() => this.handleTap(this, `/artist/${artist.slug}`)}> | ||
| <EntityHeader | ||
| imageUrl={artist.image.resized.url} |
There was a problem hiding this comment.
#minor but you may want to use a safe get here in case one of these items returns null
There was a problem hiding this comment.
That feels like a smart move. I think we should make this change, unless someone can definitely say that artist always have images.
There was a problem hiding this comment.
Artists definitely sometimes have no images, we should use get here 👍
There was a problem hiding this comment.
For future reference, we can use optional chaining instead of get now!! 🎉
artist.image?.resized.urlThere was a problem hiding this comment.
I forgot all about optional chaining! Yay!
There was a problem hiding this comment.
I think we should make the image size change that Ash recommended in the original PR, as well as the change to use get against that image url. @ashleyjelks let me know if you're planning on making those changes or if you'd prefer me to do it!
| internalID | ||
| name | ||
| image { | ||
| resized(width: 100) { |
There was a problem hiding this comment.
Per this comment on the original PR, we should update this to be either 500, or the actual screen width if it's not too much work.
|
(Though I'd also be okay with merging & then fixing those two things later, if that's your preference @ashleyjelks.) |
| expect(output).toContain("Andy Warhol") | ||
| expect(output).toContain("Joan Miro") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Test coverage looks good 👍
There was a problem hiding this comment.
(Though I'd also be okay with merging & then fixing those two things later, if that's your preference @ashleyjelks.)
Ok, let me address the img resize based on the screen width in a follow-up. The other requested changes have been made!
|
🚀 PR was released in v1.21.6 🚀 |
The work for the Featured Artists module in iOS Collections is complete, it just needed a rebase against
masteras many changes have been merged since the originalfeatured-artists-modulebranch was created. I didn't want to push to @xtina-starr 's current branch so I created a new with the latest frommasterrebased against the work @xtina-starr has already completed in #2001