feat: palette fill as a presentation prop; getInputStylePalette fn prop; #1092
Conversation
apattersonATX-HB
requested review from
pwillikins,
blambillotte,
0ces and
kbesingeryeh
apattersonATX-HB
requested review from
stephenh,
ksk5280 and
chr1sjf0x
as code owners
| @@ -2,6 +2,8 @@ import { createContext, PropsWithChildren, useContext, useMemo } from "react"; | |||
| import { GridStyle } from "src/components/Table"; | |||
| import { Typography } from "src/Css"; | |||
|
|
|||
| export type InputStylePalette = "success" | "warning" | "caution" | "info"; | |||
There was a problem hiding this comment.
Mirrors our coloredChips and does leave room for more bespoke customization in the future if the need arises
There was a problem hiding this comment.
Nice! I like giving the users "logical options" and keeping the physical color palette/colors internally within the component.
| } | ||
|
|
||
| export type PresentationContextProps = { | ||
| fieldProps?: PresentationFieldProps; | ||
| fieldProps?: Omit<PresentationFieldProps, "inputStylePalette">; |
There was a problem hiding this comment.
Not something we want set at a context level
There was a problem hiding this comment.
Makes sense, that it's not a context-level thing, but then wonder if we can find a better spot for declaring it, then PresentationFieldProps?
Hm, I guess PresentationFieldProps does seem to be "common across text fields and select fields", and it isn't used by other things that aren't getting supported in this PR... 🤔 so sgtm...
Could make your PR comment an in-source comment 🤷
There was a problem hiding this comment.
"common across text fields and select fields",
Yup! Thats why its here. Comment updated
src/inputs/SelectField.stories.tsx
Outdated
| const standardColoredSelectArgs = { | ||
| options: coloredOptions, | ||
| // @ts-ignore | ||
| getInputStylePalette, | ||
| }; | ||
|
|
||
| export const Colored = Template.bind({}); |
| /** Changes bg and hoverBg; Takes priority over `contrast`; Useful when showing many fields w/in a table that require user attention; In no way should be used as a replacement for error/focus state */ | ||
| inputStylePalette?: InputStylePalette; |
There was a problem hiding this comment.
I like this since there's been a lot of discussion around how we can show to users what changes they've made at a glance, mostly in dynamic schedules. With this we can pass something in based on a form state for example if asked
|
🕯️🕯️🕯️ Summoning @bdow to poke holes 🕯️🕯️🕯️ |
src/inputs/internal/ComboBoxBase.tsx
Outdated
| @@ -20,6 +20,8 @@ export interface ComboBoxBaseProps<O, V extends Value> extends BeamFocusableProp | |||
| getOptionMenuLabel?: (opt: O, isUnsetOpt?: boolean, isAddNewOption?: boolean) => string | ReactNode; | |||
| getOptionValue: (opt: O) => V; | |||
| getOptionLabel: (opt: O) => string; | |||
| /** Sets an input style based on the option(s) selected if `inputStylePalette` is not set */ | |||
| getInputStylePalette?: (values: V[] | undefined) => InputStylePalette | undefined; | |||
There was a problem hiding this comment.
Is this to support something like "the select field is green when the values are [Good, Better] but the select field is red when the values are [Bad, Worse]"?
Kinda wonder if just inputStylePalette as a regular prop would be fine, and let the caller do the "well sometimes is success and sometimes its warning" on their side?
There was a problem hiding this comment.
Exactly. I thought getInputStylePalette lined up with our other label getters and would cut down on extra code the caller needs to write to support something like [Good, Better] or value passes some regExp combination but its basically the same in practice so i'll remove it in favor of just relying on updating inputStylePalette values
| @@ -2,6 +2,8 @@ import { createContext, PropsWithChildren, useContext, useMemo } from "react"; | |||
| import { GridStyle } from "src/components/Table"; | |||
| import { Typography } from "src/Css"; | |||
|
|
|||
| export type InputStylePalette = "success" | "warning" | "caution" | "info"; | |||
There was a problem hiding this comment.
Nice! I like giving the users "logical options" and keeping the physical color palette/colors internally within the component.
| } | ||
|
|
||
| export type PresentationContextProps = { | ||
| fieldProps?: PresentationFieldProps; | ||
| fieldProps?: Omit<PresentationFieldProps, "inputStylePalette">; |
There was a problem hiding this comment.
Makes sense, that it's not a context-level thing, but then wonder if we can find a better spot for declaring it, then PresentationFieldProps?
Hm, I guess PresentationFieldProps does seem to be "common across text fields and select fields", and it isn't used by other things that aren't getting supported in this PR... 🤔 so sgtm...
Could make your PR comment an in-source comment 🤷
| @@ -23,10 +25,12 @@ export interface PresentationFieldProps { | |||
| errorInTooltip?: true; | |||
| /** Allow the fields to grow to the width of its container. By default, fields will extend up to 550px */ | |||
| fullWidth?: boolean; | |||
| /** Changes bg and hoverBg; Takes priority over `contrast`; Useful when showing many fields w/in a table that require user attention; In no way should be used as a replacement for error/focus state */ | |||
apattersonATX-HB
deleted the
sc-61998/single-select-field-palette-fill
branch
## [2.372.0](v2.371.2...v2.372.0) (2024-12-09) ### Features * palette fill as a presentation prop; getInputStylePalette fn prop; ([#1092](#1092)) ([794663c](794663c))
|
🎉 This PR is included in version 2.372.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
SC-61998 | Figma
Screen.Recording.2024-12-06.at.1.49.08.PM.mov
Initially the goal was to only have select fields show color but since the underlying
<TextFieldBasethat actually controls colors takes props from presentationFieldProps it was pretty easy to hook up the components starting from there. So technically our<MultiselectField also supportsgetInputStylePalette&inputStylePaletteand our<TextFieldssupportinputStylePalette` as well practically for free.