Expression Names in Column Settings Header + Refactors #2459

ada-x64 · 2023-11-30T20:11:21Z

Many things happen in this PR.

Expression names get moved to the column settings sidebar's header.
This header is editable when the attributes tab is open.
The expression editor's save, reset, and delete buttons have been moved to their own components to allow them to save and reset all attribute settings. This required a good bit of refactoring.
Column settings is now able to stay open when saving. This is accomplished by updating the viewer's "open column" message, incorporating the last opened column in the presentation, and sending that message as needed.
Expression types hadn't been updated in the perspective .d.ts file, so I fixed that
Added a TypeIcon component to replace the ::before rules on column selectors. This could be extended into a generalized Icon component.
Found a pattern to simplify parameters, i.e. including the parameters for child components as parameters of the wrapper component. This simplified several components in the settings sidebar.
Moved expression update functions to a model.

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 <[email protected]> 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 <[email protected]> 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 · 2024-01-11T17:20:06Z

I updated this PR to include about 40 new tests to cover the behavior changes. This helped me catch a number of bugs.
I was having some issues with flakiness in the tests (timing issues), but those should be fixed.
Part of that bugfixing was adding more functionality in response to the renderer.session_updated pubsub event. This occurs in the viewer component and in the column settings sidebar. There are again some issues with the plugin API which need to be addressed, but I plan on covering those as I finish #2469 .

texodus

Thanks for the PR! Looks good!

Reviewed (extensively) offline with @ada-x64 - comments are from an earlier version of this PR that I did not publish at the time but I'm leaving in for posterity!

texodus · 2023-12-31T05:29:54Z

rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar/tablist.rs

+
+#[function_component(ColumnSettingsTablist)]
+pub fn column_settings_tablist(p: &ColumnSettingsTablistProps) -> Html {
+    let match_fn = yew::use_callback(p.clone(), move |tab, p| match tab {


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.

texodus · 2023-12-31T05:33:16Z

rust/perspective-viewer/src/rust/components/column_settings_sidebar/sidebar.rs


 impl PartialEq for ColumnSettingsProps {
    fn eq(&self, other: &Self) -> bool {
-        self.selected_column == other.selected_column
+        self.selected_column == other.selected_column && self.is_active == other.is_active
    }
 }

 #[function_component]
 pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html {


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.

texodus · 2023-12-31T05:41:58Z

rust/perspective-viewer/src/rust/components/editable_header.rs

+
+    {
+        clone!(new_value, p.initial_value);
+        yew::use_effect_with(p.reset_count, move |_| new_value.set(initial_value.clone()));


If you can't pass all of the state via hook dependencies, the logic is too complicated for a function component in this project.

finos-cla-bot bot added the cla-present label Nov 30, 2023

ada-x64 mentioned this pull request Nov 30, 2023

Move Expression Names to Column Settings Header #2437

Closed

ada-x64 force-pushed the features/keep-column-settings-open branch from 812210c to 614b31c Compare December 4, 2023 16:58

ada-x64 marked this pull request as ready for review December 4, 2023 17:00

ada-x64 force-pushed the features/keep-column-settings-open branch from 614b31c to f91d3d1 Compare December 4, 2023 17:33

ada-x64 force-pushed the features/keep-column-settings-open branch from 0a9a02a to 9a9088e Compare December 18, 2023 22:49

ada-x64 mentioned this pull request Dec 18, 2023

Expression Editor Tweaks #2461

Closed

ada-x64 changed the title ~~Keep column settings open on expression changes~~ Expression Names in Column Settings Header + Refactors Dec 19, 2023

ada-x64 force-pushed the features/keep-column-settings-open branch from 9a9088e to b0dca52 Compare January 3, 2024 22:56

ada-x64 added 10 commits January 11, 2024 11:14

refactor column settings into struct component

092c9d6

retain selected tab on column manipulation; prefer passing props inst…

a34c35e

…ead of components

refactor editable header + test

37e8f93

update snapshots

4b915ba

add interaction tests

f372dbe

fix sidebar stays open on deactivation

5f58022

mv last commit's solution to viewer for general scope

0834923

fix sidebar doesn't update on aggregation/view_type change

c301e6f

fix broken and flaky tests

f2037ed

ada-x64 force-pushed the features/keep-column-settings-open branch from 6e02861 to a36f55a Compare January 11, 2024 17:19

ada-x64 force-pushed the features/keep-column-settings-open branch from a36f55a to 3f712ae Compare January 11, 2024 17:22

update snapshots

e10c625

ada-x64 force-pushed the features/keep-column-settings-open branch from 3f712ae to e10c625 Compare January 11, 2024 21:45

ada-x64 mentioned this pull request Jan 16, 2024

Add column settings render query to plugins #2500

Closed

texodus approved these changes Jan 18, 2024

View reviewed changes

texodus merged commit 4874c59 into finos:master Jan 18, 2024
13 checks passed

texodus mentioned this pull request Feb 6, 2024

Settings panel width is inconsistent for columns moved to Config #2527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression Names in Column Settings Header + Refactors #2459

Expression Names in Column Settings Header + Refactors #2459

ada-x64 commented Nov 30, 2023 •

edited

Loading

ada-x64 commented Jan 11, 2024

texodus left a comment

texodus Dec 31, 2023

texodus Dec 31, 2023

texodus Dec 31, 2023

Expression Names in Column Settings Header + Refactors #2459

Expression Names in Column Settings Header + Refactors #2459

Conversation

ada-x64 commented Nov 30, 2023 • edited Loading

ada-x64 commented Jan 11, 2024

texodus left a comment

Choose a reason for hiding this comment

texodus Dec 31, 2023

Choose a reason for hiding this comment

texodus Dec 31, 2023

Choose a reason for hiding this comment

texodus Dec 31, 2023

Choose a reason for hiding this comment

ada-x64 commented Nov 30, 2023 •

edited

Loading