Dropdown v6 Update #379
Dropdown v6 Update #379
Conversation
korbinancell
force-pushed
the
split-button
branch
from
11a1d4a
to
cac0872
Compare
|
My non-designer eyes can't tell for certain but it looks like the arrow isn't vertically aligned (feels a little closer to the top than the bottom). |
| @@ -0,0 +1,132 @@ | |||
| - [✔] Styled Systems support | |||
| - [✔] WAI-ARIA compliant | |||
components/check-box/styled.jsx
Outdated
| @@ -3,7 +3,7 @@ import { resetStyles } from '../utils'; | |||
| import { Box } from '../Box'; | |||
|
|
|||
| export const CheckboxDiv = styled(Box)` | |||
| position: absolute; | |||
| position: ${({ position }) => position || 'absolute'}; | |||
| as="a" | ||
| ref={ref} | ||
| onClick={null} | ||
| onKeyDown={null} |
There was a problem hiding this comment.
This seems to be intentional (and I'm not sure on the accessibility rules around this) but I was a little surprised that I couldn't use enter to "click" on the link in the dropdown.
| ); | ||
| const DropdownMenu = deprecateComponent( | ||
| DropdownMenuComponent, | ||
| depWarning('DropdownMenu', 'Dropdown.Menu'), |
There was a problem hiding this comment.
Definitely read this as derpWarning at first 😂
| return handleKeyboardActivate; | ||
| } | ||
|
|
||
| export function useKeyboardNavigate(onCloseMenu, onKeyboardNav) { |
| @@ -185,8 +185,8 @@ colors.select = { | |||
|
|
|||
| colors.dropdown = { | |||
| background: colors.white, | |||
| backgroundHover: colors.gray4, | |||
| foreground: colors.black, | |||
| backgroundHover: colors.gray8, | |||
There was a problem hiding this comment.
I trust this was intentional / needed.
There was a problem hiding this comment.
Discussed in Flow here: https://www.flowdock.com/app/logos/styled-ui/threads/2G-oohRCEJEeskb1UYx7h5GXr7Z
|
@RobertBolender, my current project is blocked on this component so I'm hoping to get it shipped here soon. Do you have bandwidth to review today? |
| React.isValidElement(child) && child.type.isFocusableChild | ||
| ? React.cloneElement(child, { | ||
| ref: registerItem(index), | ||
| keyboardHovered: focusedItemIndex === index, |
There was a problem hiding this comment.
(non-blocking) "keyboardHovered" strikes me as strange, is this different from "focused"?
Mouse cursors can hover, keyboards cannot.
| return ref => { | ||
| let newList = [...itemList.current]; | ||
| if (itemList.current.length < index) { | ||
| newList = newList.slice(0, index); |
There was a problem hiding this comment.
I'm not understanding the intent of this conditional statement.
Array.slice already handles the case where the "end" parameter is greater than the length of the array:
If end is greater than the length of the sequence, slice extracts through to the end of the sequence (arr.length).
There was a problem hiding this comment.
Yep, and that is what it is being used for. I was just trying to avoid some unnecessary work of slicing and extending the array.
| onFocusAway={onCloseMenu} | ||
| placement="bottom-start" | ||
| hideArrow | ||
| width={width || Styled.defaultMenuWidth} |
There was a problem hiding this comment.
If this defaultMenuWidth const is only used here and not in styled.jsx, how would you feel about just hard-coding it here?
There was a problem hiding this comment.
I usually prefer avoiding magic numbers and mixing css and js so I tried to kill two birds with one stone with keeping the styling constants in a separate file.
There was a problem hiding this comment.
I still find it strange for a const that is only used in one location to be defined in a separate location, but I'll leave it as a non-blocking objection.
| const childIndex = childrenList.findIndex(child => child.type?.childConfigComponent === name); | ||
| return childIndex !== -1 | ||
| ? [childrenList[childIndex], childrenList.filter((_, index) => index !== childIndex)] | ||
| : [null, children]; |
There was a problem hiding this comment.
If I'm reading this correctly, the return value of this getConfigChild function is an array with two elements:
- Either a single config component, if it exists, or else null
- An array of all the children components that are not the config component.
Is that accurate? Would you consider adding a comment to that effect?
No description provided.