Refactor ContextMenuWrapper #5679
Conversation
benwolfram
force-pushed
the
standardized-context-menu
branch
from
3f32494
to
fe0e70b
Compare
#12561 Bundle Size — 62.13MiB (~-0.01%).
Warning Bundle contains 52 duplicate packages – View duplicate packages Bundle metrics
|
| Current #12561 |
Baseline #12560 |
|
|---|---|---|
 Initial JS |
45.21MiB(~-0.01%) |
45.21MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
22.01% |
22.71% |
| Chunks |
30 |
30 |
| Assets |
33 |
33 |
| Modules |
4288 |
4288 |
| Duplicate Modules |
519 |
519 |
| Duplicate Code |
31.01% |
31.01% |
| Packages |
449 |
449 |
| Duplicate Packages |
52 |
52 |
Bundle size by type 2 changes 2 improvements
| Current #12561 |
Baseline #12560 |
|
|---|---|---|
| JS |
62.12MiB (~-0.01%) |
62.12MiB |
| HTML |
11.16KiB (-0.33%) |
11.2KiB |
Bundle analysis report Branch standardized-context-menu Project dashboard
|
Performance test results: |
benwolfram
force-pushed
the
standardized-context-menu
branch
3 times, most recently
from
62c5462
to
372d930
Compare
| dispatch?: EditorDispatch | ||
| data: T | ||
| renderTag?: string | ||
| children?: ReactNode |
There was a problem hiding this comment.
Normally we'd use React.PropsWithChildren but this is fine tbh
| hidden={isHidden(item)} | ||
| style={{ | ||
| height: item?.isSeparator ? 9 : 28, | ||
| display: 'flex', |
There was a problem hiding this comment.
Maybe too soon / out of scope for this refactor: but I wonder if we'd be better off with a grid here, or even a variable for the list-item layout that we want (just label, label with accesor, icon+ label, icon+label+accessor etc).
There was a problem hiding this comment.
These are valid points, but this PR is a prep step that was just about turning the thing into a functional component (I asked Ben to open this PR now so that there is less noise in the work around standardising the context menus)
|
|
||
| show( | ||
| event, | ||
| localXHack_KILLME === 'local-x-coord-KILLME' |
There was a problem hiding this comment.
again, minor - but maybe extract it to
const options = localXHack_KILLME === 'local-x-coord-KILLME'
? { position: { x: event.nativeEvent.offsetX, y: event.nativeEvent.pageY } }
: undefined
show(event, options)
| } | ||
| const renderItem = useCallback( | ||
| (item: Item<T>, index: number) => { | ||
| return ( |
There was a problem hiding this comment.
how easy/hard will it be to extract it to its own component instead of a callback?
There was a problem hiding this comment.
I probably should do this.. I'll update!
There was a problem hiding this comment.
So... this felt easy and like a no brainer for organization. But I feel like there are just hidden traps waiting for me in this component 😂. I spent all afternoon trying to get it and making it a new component broke the 'Paste' functionality and tests. So I've reverted that.
If you think it's important, maybe we can follow up later.
benwolfram
force-pushed
the
standardized-context-menu
branch
2 times, most recently
from
8c7424b
to
3d13512
Compare
benwolfram
force-pushed
the
standardized-context-menu
branch
from
3d13512
to
9ff7a86
Compare
Problem:
Modernizing this file to show it some love in preparation of work to improve context menus and upgrade react-contexify, ie. keyboard navigation.
Fix:
Updating to modern react hooks and functional components.
Only note, is that I chose to remove the memoization (formerly
shouldComponentUpdate, replace withmemoin one of my earlier commits). I don't see a performance impact from this and it's better to not pre-emptively memoize. Typescript wasn't able to properly infer the generic types when wrapping with memo (see the commit related to removing memo).s/o to @Rheeseyb for the help
Manual Tests:
I hereby swear that:
Relates to #5643