fix(component): performance improvements for Dropdown/Select components #475
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgemoya
force-pushed
the
select-performance
branch
3 times, most recently
from
aff89fc to
d6b06b9
Compare
jorgemoya
force-pushed
the
select-performance
branch
from
d6b06b9 to
2fb2c70
Compare
chanceaclark
reviewed
Dec 9, 2020
| } | ||
| </Reference> | ||
| isValidElement(toggle) && | ||
| cloneElement<React.HTMLAttributes<any> & React.RefAttributes<any>>(toggle as any, { |
There was a problem hiding this comment.
This can be reduced to this:
Suggested change
| cloneElement<React.HTMLAttributes<any> & React.RefAttributes<any>>(toggle as any, { | |
| cloneElement(toggle, { |
| import { StyledBox } from './styled'; | ||
| import { DropdownItem, DropdownLinkItem, DropdownProps } from './types'; | ||
|
|
||
| import { DropdownItemGroup } from '.'; |
There was a problem hiding this comment.
Let's import this from ./types above.
| isAction = false, | ||
| ...props | ||
| }) => { | ||
| type Items = DropdownItem | DropdownLinkItem | SelectOption<unknown> | SelectAction; |
There was a problem hiding this comment.
I think you can add a generic to Items
Suggested change
| type Items = DropdownItem | DropdownLinkItem | SelectOption<unknown> | SelectAction; | |
| type Items<T> = DropdownItem | DropdownLinkItem | SelectOption<T> | SelectAction; |
|
|
||
| export const List = memo( | ||
| forwardRef<HTMLUListElement, ListProps>((props, ref) => <StyleableList {...props} forwardedRef={ref} />), | ||
| forwardRef<HTMLUListElement, ListProps<unknown>>((props, ref) => <StyleableList {...props} forwardedRef={ref} />), |
There was a problem hiding this comment.
Can we add a comment on why we need this unknown?
jorgemoya
force-pushed
the
select-performance
branch
from
90e1459 to
95f11b1
Compare
chanceaclark
approved these changes
Dec 10, 2020
| @@ -3,7 +3,7 @@ import { theme as defaultTheme } from '@bigcommerce/big-design-theme'; | |||
| import { hideVisually } from 'polished'; | |||
| import styled, { css } from 'styled-components'; | |||
|
|
|||
| import { StyledButton } from '../Button/styled'; | |||
| import { StyleableButton } from '../Button/Button'; | |||
There was a problem hiding this comment.
🍹
Suggested change
| import { StyleableButton } from '../Button/Button'; | |
| import { StyleableButton } from '../Button/private'; |
jorgemoya
force-pushed
the
select-performance
branch
from
403e2d8 to
26edce4
Compare
|
We're looking for one more set of eyes @bigcommerce/frontend |
deini
approved these changes
Dec 14, 2020
| { | ||
| name: 'eventListeners', | ||
| options: { | ||
| scroll: isOpen, |
|
There is an issue with Downshift and memoized components. I have opened a PR for a potential fix, I would advise not to merge until the fix is released. downshift-js/downshift#1205 |
jorgemoya
force-pushed
the
select-performance
branch
from
78a8d72 to
e434adb
Compare
jorgemoya
force-pushed
the
select-performance
branch
from
e434adb to
4672aab
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Long time coming, but we noticed every single item would rerender whenever hovering an option, so we knew performance could be improved.
This changes fix that by memoizing the props that were triggering a rerender, thus only items with true changes will rerender. I added some props to some components for feature parity (like groups to MultiSelect). A lot of things were moved around, hoping this helps with clarity of these components.
Some area of opportunity:
as. Let me know if we can fix them some other way!useMultipleSelectiondownshift hook.Screenshots