Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| end, | ||
| startIconAccessibilityLabel = 'Back', | ||
| clearIconAccessibilityLabel = 'Clear text', | ||
| borderRadius = 1000, |
There was a problem hiding this comment.
Also unlocking custom border radius for search input
| export const scales = { | ||
| regular: 56, | ||
| compact: 40, | ||
| }; |
There was a problem hiding this comment.
SearchInput (at least on web) is still hardcoded with height, I believe this is already fixed in v9
|
|
||
| return typeof singleValueContent === 'string' ? ( | ||
| <Text align={align} color={hasValue ? 'fg' : 'fgMuted'} ellipsize="tail" font="body"> | ||
| <Text align={align} color={hasValue ? 'fg' : 'fgMuted'} ellipsize="tail" font={inputFont}> |
There was a problem hiding this comment.
Feels slightly funky to use inputFont here but this matches visually with the same spot on TextInput and Combobox.
There was a problem hiding this comment.
hmm yeah def funky naming for Select specifically - makes sense in the context of combobox
There was a problem hiding this comment.
We could call it controlFont for Select and then map the property for combobox's NativeInput, any thoughts there?
There was a problem hiding this comment.
@hcopp what about just font for Select? Could that work for Combobox too and we just document that it maps to both the values and text input elements? If they needed to break from that convention (seems uncommon) they could provide a custom component
|
|
||
| return typeof singleValueContent === 'string' ? ( | ||
| <Text align={align} color={hasValue ? 'fg' : 'fgMuted'} ellipsize="tail" font="body"> | ||
| <Text align={align} color={hasValue ? 'fg' : 'fgMuted'} ellipsize="tail" font={inputFont}> |
There was a problem hiding this comment.
hmm yeah def funky naming for Select specifically - makes sense in the context of combobox
| * Typography font token used for typed search input text in select controls that render inputs. | ||
| * @default body | ||
| */ | ||
| inputFont?: ThemeVars.Font; |
There was a problem hiding this comment.
when should we be using ResponsiveProps? I'm worried we're kind of forgetting about this pattern. I believe font on its own as a styled prop is responsive
There was a problem hiding this comment.
We could switch NativeInput from to which would unlock responsive props, however one nuance is that a few html built in props (like color) would have gone from accepting any normal CSS color value to ThemeColor. I don't see any uses of this in SourceGraph but could be an issue.
Do you think that is worth doing?
There was a problem hiding this comment.
probably not worth the hassle - good call out on potentially breaking type change, but why can't you just do:
inputFont?: ResponsiveProps<ThemeVars.Font>; // i forgot the actual type name sorry!
There was a problem hiding this comment.
tbh its not super important to me right now, im fine if youw ant to keep it as is but we should return to this topic in eng sync: props of type ThemeVars.X on web that dont match the behavior of their associated Style props
3 participants
What changed? Why?
This PR enables extra customization of our inputs through
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false