feat(Dropdown): update styles for dark and hc Teams themes #1299
Conversation
-fixed issues in light theme
-fixed rounded/corner borders of the dropdown and list
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason and
miroslavstastny
as code owners
mnajdova
changed the title
# Conflicts: # packages/react/src/themes/teams-high-contrast/components/Input/inputVariables.ts # packages/react/src/themes/teams/components/Dropdown/dropdownVariables.ts
-added isFromKeyboard -TODO: update focused styles
-implemented styles for selected items
# Conflicts: # packages/react/src/themes/teams-dark/componentVariables.ts # packages/react/src/themes/teams-high-contrast/componentVariables.ts
-added icon filled on hover
mnajdova
changed the title
packages/react/src/themes/teams/components/Dropdown/dropdownItemStyles.ts
Outdated
Show resolved
Hide resolved
|
Click anywhere on the dropdown doesn't seem to work anymore
|
Codecov Report
@@ Coverage Diff @@
## master #1299 +/- ##
==========================================
- Coverage 72.33% 72.16% -0.18%
==========================================
Files 759 762 +3
Lines 5694 5734 +40
Branches 1688 1702 +14
==========================================
+ Hits 4119 4138 +19
- Misses 1569 1590 +21
Partials 6 6
Continue to review full report at Codecov.
|
|
The issue with the trigger button is resolved, everything works as expected now. |
# Conflicts: # packages/react/src/components/Dropdown/DropdownItem.tsx
packages/react/src/themes/teams-dark/components/Dropdown/dropdownVariables.ts
Outdated
Show resolved
Hide resolved
| @@ -453,6 +459,17 @@ class Dropdown extends AutoControlledComponent<WithAsProp<DropdownProps>, Dropdo | |||
| const { triggerButton } = this.props | |||
| const content = this.getSelectedItemAsString(this.state.value) | |||
|
|
|||
| const toggleButtonProps = getToggleButtonProps({ | |||
There was a problem hiding this comment.
is toggle and trigger button the same thing? Seems that yes, given where props of toggleButton are applied to. Would suggest to maintain naming consistency in this case
There was a problem hiding this comment.
getToggleButtonProps as a method comes from Downshift, and we are using this method on the triggerButton - main area of the single selection Dropdown, and the toggleIndicator, the arrow on the right side of the Dropdown. Can you please let me know what should be renamed in this case?
There was a problem hiding this comment.
given that method is named renderTriggerButton, as well as that props contained in scoped variable toggleButtonProps are applied to trigger button only in the context of this method, I would rename it as triggerButtonProps - yes, we have getToggleButtonProps as method from Downshift, but in the context of our method, where everything is scoped to this method, I would expect to see everything related to triggerButton, accordingly
There was a problem hiding this comment.
Got it, will rename the const to triggerButtonProps.
packages/react/src/themes/teams-high-contrast/components/Dropdown/dropdownStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Dropdown/dropdownItemStyles.ts
Outdated
Show resolved
Hide resolved
| }) | ||
|
|
||
| const { onClick, onFocus, onBlur, onKeyDown, ...restToggleButtonProps } = toggleButtonProps | ||
|
|
||
| return ( | ||
| <Ref innerRef={this.buttonRef}> | ||
| {Button.create(triggerButton, { |
There was a problem hiding this comment.
this is another good example of why we should refrain from blindly passing all calculated props to any slot's content - we might imagine what it would be if regular <button />, or even any other Stardust component type which is different from Button will be provided here
Fixes #1215 - Dropdown theming.
Fixes #1329 - onClick is breaking the triggerButton behavior.
Fixes #1254 - Dropdown event handlers should be refactored.
In order for the styles to applied correctly, I've added the following things:
selectedprop for theDropdownItem, which is set to true from theDropdown, if the selection is single, and the item is the selected oneisFromKeyboardprop for theDropdownItem, which is set from theDropdown, based on the type of event received from Downshift, when the item is set to activeisFromKeyboardstate for theDropdownThins that will be addressed in separte PRs:
invertedprop to the Dropdownselectedicon for the selected list item (may be implemented differently)