[BOOKINGSG-9157] refactor: migrate dropdown-list to linaria and v4 tokens#1107
[BOOKINGSG-9157] refactor: migrate dropdown-list to linaria and v4 tokens#1107ziggyzet wants to merge 5 commits intopre-release/v4from
Conversation
There's a different result on `RangeSelect`
|
| ); | ||
| max-height: min( | ||
| 27rem, | ||
| var(--fds-internal-dropdown-list-container-available-height, 9999px) |
There was a problem hiding this comment.
Can check whether this is related to the one in ElementWithDropdown:191
|
|
||
| useApplyStyle(nodeRef, { | ||
| ...floatingStyles, | ||
| ...containerWidthStyle, |
There was a problem hiding this comment.
useApplyStyle will applied the value as string and containerWidthStyle seems to be a pure number, this will make the resulting value missing unit.
| // ========================================================================= | ||
|
|
||
| useApplyStyle(nodeRef, { | ||
| ...floatingStyles, |
There was a problem hiding this comment.
Inside floatingStyles, we have zIndex and useApplyStyle currently not able to map camelCase zIndex to z-index. We likely don't have it in the result
| <BasicInput | ||
| ref={ref} | ||
| value={value} | ||
| variant={variant} |
There was a problem hiding this comment.
Shouldn't we keep this to handle small variant font?
| export const secondaryText = css` | ||
| ${tokens.secondaryText.maxLines}: 2; | ||
| color: ${Colour["text-subtlest"]}; | ||
| width: 100%; | ||
| overflow-wrap: break-word; | ||
| `; |
There was a problem hiding this comment.
maxLines token, width, and overflow-wrap can be extracted to a general text class name
| xSpacing: "--fds-internal-dropdown-list-container-x-spacing", | ||
| availableWidth: "--fds-internal-dropdown-list-container-available-width", | ||
| availableHeight: "--fds-internal-dropdown-list-container-available-height", |
There was a problem hiding this comment.
| xSpacing: "--fds-internal-dropdown-list-container-x-spacing", | |
| availableWidth: "--fds-internal-dropdown-list-container-available-width", | |
| availableHeight: "--fds-internal-dropdown-list-container-available-height", | |
| xSpacing: "--fds-internal-dropdownList-container-xSpacing", | |
| availableWidth: "--fds-internal-dropdownList-container-availableWidth", | |
| availableHeight: "--fds-internal-dropdownList-container-availableHeight", |
| flex-shrink: 0; | ||
| height: 1lh; | ||
| width: 1rem; |
There was a problem hiding this comment.
Can extract these three attributes as a general indicator class name, then have another variants for adding the color
| cursor: pointer; | ||
| overflow: hidden; | ||
| color: ${Colour["text-primary"]}; | ||
| font-size: inherit; | ||
| ${Font["body-baseline-semibold"]} |
There was a problem hiding this comment.
Similar to selectAllButton, can have another class name for padded one
| const containerWidthStyle: React.CSSProperties = width | ||
| ? { width } | ||
| : matchElementWidth && elementWidth | ||
| ? { width: elementWidth } | ||
| : {}; |
There was a problem hiding this comment.
Can we avoid having a nested ternary, it has bad readability
| className={clsx( | ||
| styles.listItem, | ||
| disabled && styles.listItemDisabled, | ||
| active && selected && styles.listItemActiveSelected, |
There was a problem hiding this comment.
Can't we have selected as the only condition?
| $disabled={!selected && hasSelectedMax} | ||
| className={clsx( | ||
| styles.listItem, | ||
| disabled && styles.listItemDisabled, |
There was a problem hiding this comment.
Should also check when it is not active.
On this limit selection example of MultiSelect, disabled option still have that active background on hover
2 participants
Checklist
classNameis chained correctly withclsx- [ ] Updated Storybook documentation- [ ] Listed breaking changesAdditional information
calc(var(<TOKEN>))useApplyStyleshook is a mustlanguage-switcherVirtuosocomponent while running the unit test. It doesn't fail the tests and haven't made changes to fix it.