[Dropdown] Apply box-shadow changes #338
Conversation
…make typings consistent
@benkolde Tagging you for a design review before an engineer for a code review because this behavior in particular seems suboptimal. Since our focus outline is smaller than the new |
| .create( | ||
| <MobileDropdown | ||
| onSelectChange={() => undefined} | ||
| borderRadius="4px" |
There was a problem hiding this comment.
It's a little annoying to have to provide borderRadius for the Jest tests--however, I thought it was cleaner to add the optional argument w/ default at the parent component Dropdown, instead of keep the value optional there (without a default) and setting the default on both MobileDropdown and DesktopDropdown.
| max-height: ${SPACER.x4large}; | ||
|
|
||
| border: 1px solid ${COLORS.border}; | ||
| ${dropdownBorderRadius}; |
There was a problem hiding this comment.
This line is the only change--the diff is larger due to indentatio.
| optionsContainerMaxHeight: string; | ||
| textAlign: 'left' | 'center'; | ||
| value?: string; | ||
| }; | ||
|
|
||
| const DesktopDropdown = ({ |
There was a problem hiding this comment.
Larger diff is from alphabetizing--only change in this file is adding borderRadius (and shouldBeFullyRounded) logic.
| }; | ||
|
|
||
| const Dropdown = (props: DropdownProps) => { |
There was a problem hiding this comment.
CHANGELOG:
- Move
defaultPropsto ES6 defaults.- Improved type-inference from relying on language built-ins without working out
defaultPropslimitations.- e.g., the compiler can infer that even though
borderRadiusis optional, since we're providing it with defaults when we pass it intoDesktopDropdownandMobileDropdownwe can type it there as required.
- e.g., the compiler can infer that even though
- There's still some inconsistencies: we're passing an ES6 default in
MobileDropdowneven though we typed it as a required prop. I'm not sure if that's just due to making things easier to test (related comment: https://github.com/curology/radiance-ui/pull/338/files#r488011625) but this PR already has enough ancillary changes so I didn't want to change too much.
- Improved type-inference from relying on language built-ins without working out
- Modify type of
valueinOptionType- We rely on a falsy default for our dropdown: changing the type from
stringtostring | undefinedreflects this. - There's some inconsistency in our falsy default--
nullvs.undefined. The type definition for<option>prefersundefinedinstead ofnull(OptionHTMLAttributes<HTMLOptionElement>.value?: string | number | readonly string[] | undefined) so this PR prefers that when making changes.
- We rely on a falsy default for our dropdown: changing the type from
| onSelectChange: (event: React.ChangeEvent<HTMLSelectElement>) => void; | ||
| options: OptionType[]; | ||
| textAlign?: 'left' | 'center'; | ||
| value?: string | number | undefined; | ||
| }; | ||
|
|
||
| const MobileDropdown = ({ |
There was a problem hiding this comment.
CHANGELOG:
- Change default
valueandoptions[number].valuetype fromnulltoundefinedto conform to HTML attribute types. - Remove defaultProps for
onSelectChange- I'm assuming this was included to make the tests easier--it's a required TypeScript type so this makes things more consistent.
- Sets
textAlignTypeScript type as optional to reflect that we have an ES6 default.- This isn't consistent with parent
Dropdownbehavior but I didn't want to remove this functionality. All sub-component usage goes throughDropdown.
- This isn't consistent with parent
What and Why
clickableandclickableHoverstyles to match what design has in Figma.border-radiusstyling for the Dropdown component, which previously had noborder-radius.border-radius: 4pxsince we rely on native select dropdown for functionality.border-radius: 4pxunless it is open, at which point the following occurs:border-radius: 4px 4px 0 0, shorthand for setting the top-left and top-right border radius.border-radius: 0 0 4px 4px, shorthand for setting the bottom-left and bottom-right border radius.borderRadiusprop, so that the aboveborder-radiusvalues are programmable, with4pxas the default.Clickable changes
Before:
After:
**Action button changes (as a result of
clickablechanges)``Before:
After:
Dropdown changes
Before:
After:
Next Steps
clickableandclickableHoverstyle changes will also be used in future PRs updating ourAccordionandOptionButtoncomponent.