Feature - Select: allow sub label #24
Conversation
| export const select = () => { | ||
| const items: Array<SelectItem> = [ |
There was a problem hiding this comment.
moved here, so it's shown in storybook example.
|
Travis automatic deployment: |
|
Travis automatic deployment: |
… into issue-20-select-sublable
|
Travis automatic deployment: |
src/inputs/Select/index.tsx
Outdated
| @@ -64,7 +80,16 @@ function Select({ items, activeItemId, onItemClick, id, ...rest }: Props) { | |||
| return ( | |||
| <MenuItem value={i.id} key={i.id}> | |||
| {i.iconUrl && <IconImg alt={i.label} src={i.iconUrl} />} | |||
There was a problem hiding this comment.
what if it fails to fetch an icon?
There was a problem hiding this comment.
this is a case for many tokens in safe multisig, we need to be able to specify a default placeholder
There was a problem hiding this comment.
you can check my implementation for safe ethswap app https://github.com/mikheevm/safe-app-tokenswap/blob/master/src/components/TokenIcon.tsx
There was a problem hiding this comment.
yes, makes sense, but what image should we use, this is a generic component, so I would guess that using a token image as a placeholder won't be useful in many cases.
Perhaps we can add a new prop, defaultPlacehoder, and make it be a react component.
wdty?
There was a problem hiding this comment.
it doesn't need to be a react component but an image
There was a problem hiding this comment.
but if they pass an URL, we can have the same problem. Or am I missing something?
There was a problem hiding this comment.
if they pass a react component which uses an invalid image url we can have the same problem
There was a problem hiding this comment.
there's no fully bulletproof solution
There was a problem hiding this comment.
ok, let's accept another URL to load in case the main URL fails.
src/inputs/Select/index.tsx
Outdated
| <Text size="sm" color="secondary" strong> | ||
| {i.title} | ||
| </Text> | ||
| )} | ||
| <Text size="sm" color="text"> | ||
| {i.label} | ||
| </Text> |
There was a problem hiding this comment.
The color seems to be the same even different props are passed, is that expected?
There was a problem hiding this comment.
do you mean the text color? Yes, it's hardcoded, do you think Title/Label colors should be customized via props?
There was a problem hiding this comment.
Title:
<Text size="sm" color="secondary" strong>
Label:
<Text size="sm" color="text">
one has color secondary and another one has color text, but the rendered colors are the same, is this expected? In general I'd expect secondary color to be more faded compared to the main color
There was a problem hiding this comment.
Also I think it makes more sense to make label a primary text (so label = title), and add a prop for secondary text (would replace label)
There was a problem hiding this comment.
that will be a "breaking visual" change. perhaps we can rename it to mainLabel instead of title.
There was a problem hiding this comment.
wdyt @mikheevm ?
There was a problem hiding this comment.
currently it doesn't make sense
imagine I want to display this
{
"name": "ETH",
"balance": "0.3 ETH"
}
so in this case, title = name, label = balance
but balance should not be used as a label, label should equal to name prop. And the balance is just a secondary text
There was a problem hiding this comment.
we should get rid of title and instead use label/secondary text
|
@mikheevm changes were done! |
|
Travis automatic deployment: |
closes #20