feat(home view): add icons to navigation pills #11528
Conversation
|
Seeing a large number of unrelated failures, will have to investigate… |
anandaroop
force-pushed
the
anandaroop/ONYX-1484-quick-links-icons
branch
from
e4a978c to
9b76941
Compare
| const SUPPORTED_ICONS: Record<string, ComponentType> = { | ||
| FollowArtistIcon, | ||
| AuctionIcon, | ||
| HeartIcon, | ||
| } |
There was a problem hiding this comment.
We may want to launch with a larger set of icons supported right at the outset.
| import { | ||
| AuctionIcon, | ||
| Box, | ||
| Flex, | ||
| FlexProps, | ||
| FollowArtistIcon, | ||
| HeartIcon, | ||
| Pill, | ||
| Skeleton, | ||
| Spacer, | ||
| Text, | ||
| useSpace, | ||
| } from "@artsy/palette-mobile" |
There was a problem hiding this comment.
I believe only the name would change, maybe adding the color using the useColor hook, but I don't think it would take that much effort to change it after the refactor :)
| if (name && name in SUPPORTED_ICONS) { | ||
| const Icon = SUPPORTED_ICONS[name] |
There was a problem hiding this comment.
I'm wondering if using something like switch ... case would be better here to make sure that if the name of the ion changes we're safe
switch ( name ) {
case 'HeartIcon':
return HeartIcon
case 'AuctionIcon':
return AuctionIcon
...
default:
return null
}
example after updating icons:
switch ( name ) {
case 'HeartIcon':
return HeartIconNew
case 'AuctionIcon':
return AuctionIconNew
...
default:
return null
}
There was a problem hiding this comment.
This is a great point.
Just glancing at the first batch of icons we're looking at something like…
| artsy/palette-mobile | artsy/icons |
|---|---|
| today, in this PR | upcoming, after refactor |
AuctionIcon |
Gavel |
HeartIcon |
HeartStrokeIcon |
FollowArtistIcon |
🚫 (missing?!) |
…with more to come if we add even more supported icons.
I think that instead of a switch statement, the existing mapping on L190 already provides the place to do this:
// today
const SUPPORTED_ICONS: Record<string, ComponentType> = {
FollowArtistIcon,
AuctionIcon,
HeartIcon,
}// re-mapped in the future
const SUPPORTED_ICONS: Record<string, ComponentType> = {
"FollowArtistIcon": FollowArtistIconTBD,
"AuctionIcon": Gavel,
"HeartIcon": HeartStrokeIcon,
}So I'm inclined to leave this as-is, unless you foresee any problems with that.
There was a problem hiding this comment.
Even better might be to do the (inverted) mapping now, and then simplify it in the future once that refactor lands:
// mapped today
const SUPPORTED_ICONS: Record<string, ComponentType> = {
"FollowArtistIcon": FollowArtistIcon,
"Gavel": AuctionIcon,
"HeartStrokeIcon": HeartIcon ,
}// un-mapped in the future
const SUPPORTED_ICONS: Record<string, ComponentType> = {
FollowArtistIcon,
AuctionIcon,
HeartIcon,
}That might make things tidier for future-us 🤷🏽
There was a problem hiding this comment.
But I would only want to do that^ if we are 99% sure that refactor is going to proceed. (Wdyt @MrSltun?)
anandaroop
force-pushed
the
anandaroop/ONYX-1484-quick-links-icons
branch
from
9b76941 to
cdc4852
Compare
Note
Depends on artsy/metaphysics#6423
This PR resolves ONYX-1464
Description
Consumes the new
iconfield of theNavigationPillMP type in order to render icons in the Quick Links section of the home view.PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.