Expression Names in Column Settings Header + Refactors #2459
Conversation
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
812210c to
614b31c
Compare
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
614b31c to
f91d3d1
Compare
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
0a9a02a to
9a9088e
Compare
ada-x64
changed the title
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
9a9088e to
b0dca52
Compare
split sidebar into components new exprs keep edited names error on empty; don't autosave on blur tests better state mgmt + ui tweaks Remove column selector close button when column settings is open. Ignore failing promise in code editor. update snapshots empty name is expression placeholder Co-authored-by: Davis Silverman <sinistersnare@users.noreply.github.com> use Expression::new with optional aliases empty header styling empty expression names add test expression editor style tweaks udpate tests keep col settings open exprs always show expr icon Close expressions when necessary Co-authored-by: Davis Silverman <sinistersnare@users.noreply.github.com> rerender when switching active state reset expression editor buttons on save rm column settings viewer element attribute use presentation to track last opened tab ensure tablist is populated correctly on rename keep expression editor open on save always send update on column change update tests assure column settings is open before editing or renaming columns - Move reset and save button to their own components - Move state to sidebar component - Simply child components - Reset and save now work for both expr name and value - update tests
…ead of components
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
6e02861 to
a36f55a
Compare
|
I updated this PR to include about 40 new tests to cover the behavior changes. This helped me catch a number of bugs. |
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
a36f55a to
3f712ae
Compare
ada-x64
force-pushed
the
features/keep-column-settings-open
branch
from
3f712ae to
e10c625
Compare
|
|
||
| #[function_component(ColumnSettingsTablist)] | ||
| pub fn column_settings_tablist(p: &ColumnSettingsTablistProps) -> Html { | ||
| let match_fn = yew::use_callback(p.clone(), move |tab, p| match tab { |
There was a problem hiding this comment.
Callback is for things connect to DOM/user events only, they are not a portable function abstraction. If you find yourself calling Callback::emit for anything other than composing Callbacks, or needing to specify the Callback second generic (return type) parameter, consider a different approach.
In this case, this abstraction is quite tangled. match_fn here closes over a clone of the entire ColumnSettingsTablistProps, which it passes into its child which is then called during render (not from a DOM or user event). Please take a look at ScrollPanel and ScrollPanelItem for an example of how to use typed children property, then this abstraction can go away and the choice of ColumnSettingsTab can be made in a TabListItem typed child literal int he parent SideBar directly.
| } | ||
|
|
||
| #[function_component] | ||
| pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html { |
There was a problem hiding this comment.
This needs to be a struct component. Struct components are the standard for this project currently and need to be used for anything that is long enough to defy simple code review, e.g. when a function component grows beyond ~100 loc (without playing code golf), or when more then one use_state or use_memo stateful hook is necessary.
|
|
||
| { | ||
| clone!(new_value, p.initial_value); | ||
| yew::use_effect_with(p.reset_count, move |_| new_value.set(initial_value.clone())); |
There was a problem hiding this comment.
If you can't pass all of the state via hook dependencies, the logic is too complicated for a function component in this project.
2 participants
Based on #2461, #2437
Many things happen in this PR.