-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data picker - code cleanup #5705
Conversation
…to fix/data-picker-cleanup
#12538 Bundle Size — 62.13MiB (~-0.01%).
Warning Bundle contains 52 duplicate packages – View duplicate packages Bundle metrics
|
Current #12538 |
Baseline #12534 |
|
---|---|---|
Initial JS | 45.21MiB (~-0.01% ) |
45.21MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 22.71% |
21.44% |
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 #12538 |
Baseline #12534 |
|
---|---|---|
JS | 62.12MiB (~-0.01% ) |
62.13MiB |
HTML | 10.94KiB (-0.34% ) |
10.97KiB |
Bundle analysis report Branch fix/data-picker-cleanup Project dashboard
background: currentExpressionExactMatch ? colorTheme.primary.value : undefined, | ||
color: currentExpressionExactMatch ? colorTheme.white.value : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the data picker UX isn't final yet, I didn't fix this up
…to fix/data-picker-cleanup
return element | ||
} | ||
const replacementPath = action.replacementPath | ||
if (replacementPath.type === 'replace-child-with-uid') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a switch statement here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for calling it out, neither is ideal so I refactored it to a switch + helper functions in 0b1d170
Problem
The data picker control is structured in a way that makes it hard to use for use cases other than picking data for properties
Fix
This PR refactors the data picker so that both the variables it displays and the callback that's called when a variable is clicked are passed in from the call site. This way, we can provide the variables that make sense for properties, map expression values or child nodes.
Commit Details
UpdateMapExpression
action is removed and its functionality is rolled intoReplaceElementInScope
ElementReplacementPath
to cater to properties and mapped expression, and theReplaceElementInScope
action is updated to interpret these modesuseDataPickerButton
andDataPickerComponent
are refactored so that the variable options to display and the callback that's called when an option is picked are passed in from the outsideDataPickerType
anduseDataPickerButtonListSource
are removed, because the functionality they implemented is superseded by passing in the variables and the callbackOut of scope
#5684 mentions updating the logic that gets the runtime value of variables. This PR doesn't directly fix that, but since the logic related to variables is contained in
useVariablesInScopeForSelectedElement
, this PR unblocks making this change (because with this, the various call sites in different contexts can pass in whatever variables they deem fit).Manual Tests
I hereby swear that:
Related to #5684