Move styles to sidebar #2366

ada-x64 · 2023-09-12T20:06:43Z

This PR moves the datagrid style custom elements to a new tabbed sidebar called the column settings sidebar.

All active columns now have an edit button which will open the column settings. Expression columns can be edited when inactive as well. Plugins other than the datagrid will have a "styles not implemented" placeholder until we eventually add styles to them.

The PR also adds significant playwright scaffolding for UI tests that are not based on snapshots. This includes updating the yarn file to use the latest version of Playwright.

Create tabbed sidebar
Move ExprTK editor to tabbed sidebar
Move Yew components to tabbed sidebar
Hook up plugins and sidebar
Make the style buttons on the datagrid plugin open the style sidebar
NTH: Datagrid should autoscroll to currently selected column when editing.
Remove extraneous files
Add tests

KNOWN ISSUES:

Column settings sidebar is drawn over the plugin, so it can overlap the focused column.
Edit button is a bit sticky. Select a column, change something, then select another column. The edit button should move.
Mousedown on the active columns list edit button always opens the sidebar, even if you drag the selected column.

Co-authored-by: Davis Silverman <sinistersnare@users.noreply.github.com> Co-authored-by: Timothy Bess <timbessmail@gmail.com> Co-authored-by: Broch Stilley <brochington@gmail.com> Fix datagrid bugs

add splitby tests

texodus

Thanks for the PR! Looks good!

This looks great overall, and is a really cool feature to boot.

One issue we do need to address is default_config in plugin_config being persisted. This is likely a bug introduced in this PR, and while its mostly innocuous it is also a breaking change to the package's public API, specifically the structure of the config returned by save(). Perspective's config structure is frequently persisted in hard-to-update systems like Database tables and may be encoded in non-human-readable format, so it's important we conserve changes to this data structure.

IMO we should fix this before we merge this PR, so we can revert the test changes which assert the modified behavior.

texodus · 2023-09-20T01:26:03Z

packages/perspective-jupyterlab/test/jupyter/widget.spec.js

@@ -214,6 +214,7 @@ describe_jupyter(
                });

                // Check default config
+                config.plugin_config.default_config = {};


This is a bug, we should not be persisting default_config, which is a property that determines the settings for plugin_config when a field is not specified.

In order to fix this I added a default_config getter to the plugins which act as an opt-in to the style API. If this is not set, the plugins won't get styled. See style_tab.rs

texodus · 2023-09-20T02:04:06Z

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

+#[function_component]
+pub fn ColumnSettingsSidebar(p: &ColumnSettingsProps) -> Html {
+    let column_name = match p.selected_column.clone() {
+        ColumnLocator::Expr(maybe_name) => maybe_name.unwrap_or("New Column".to_string()),


This should be an abort - returning "New Column" shouldn't ever be reached but if it does, this error condition is just kicked down the road here where it may be difficult to debug.

ColumnLocator::Expr(None) is used when creating a new Expression column. However, we shouldn't be hardcoding the name, so I used the same function as in the expression_editor to generate the name.

texodus · 2023-09-20T02:05:22Z

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

+            {title}
+            on_close={p.on_close.clone()}
+            id_prefix="column_settings"
+            icon={html_template!{


IMO this is overkill - the icon data is metadata that can be passed as a prop, it's never going to be some other DOM structure, in this case the id attribute.

texodus · 2023-09-20T02:13:45Z

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

+                ColumnLocator::Expr(None) => save_expr(v, p),
+                ColumnLocator::Expr(Some(alias)) => update_expr(alias, &v, p),
+                _ => {
+                    tracing::error!("Tried to save expression in bad state!")


AttributesTab cannot currently be instantiated for non expression columns (which this error alludes to). We can represent this illegal state in the type system and validate it at compile time by not using ColumnLocator in props, rather just Option<String>.

texodus · 2023-09-20T02:15:35Z

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

+        p.clone(),
+    );
+
+    let alias = yew::use_memo(


This doesn't need to be a use_memo, just calculate it in the component body. In this case it's not even conserving the clone() as you must clone into the closure.

texodus · 2023-09-20T02:19:48Z

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

+/// This function sends the config to the plugin using its `restore` method
+fn send_config<T: Serialize + Debug>(
+    renderer: Renderer,
+    presentation: Presentation,


If you pass these by reference, they will not need to be cloned at the call site.

The main place we want to clone() + move into a function is when spawning the first frame on a new async stack, as the async frame cannot reference the spawning stack.

texodus · 2023-09-20T02:23:05Z

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

+            let view = view.unchecked_into();
+            elem.update(&view, None, None, false)
+                .await
+                .expect("Unable to call update!");


While we don't care if this fails, generally panicking in Yew is bad, as it can leave the app in an inconsistent state. ApiFuture::spawn expects a ApiResult return type which is compatible with update and correctly routes this failure to the Promise.reject() method, both reporting the failure in the console properly and avoiding panic.

texodus · 2023-09-20T02:30:39Z

rust/perspective-viewer/src/rust/custom_elements/viewer.rs

+    /// The Viewer component will then send a
+    /// "perspective-toggle-column-settings" event to the plugin with the
+    /// currently selected column. The plugin should respond to the event to
+    /// highlight the selected column appropriately.


These are the API docs which are published to https://perspective.finos.org/docs/obj/perspective-viewer/ , and unlike the rest of the repo, they should be written from the point-of-view of a developer using <perspective-viewer> in their application, not a developer working on the Perspective codebase itself.

I updated the docs to something I think is more user-friendly. Let me know if it needs more changes.

texodus · 2023-09-20T02:34:21Z

rust/perspective-viewer/src/rust/session/metadata.rs

+            })
+        } else {
+            // new expr
+            Some(ColumnLocator::Expr(None))


Under what conditions does this function return None? I think this is a bit over-wrapped.

This should return None when there is not a currently existing column. I removed the Expr(None) case.

ada-x64 · 2023-09-20T17:45:04Z

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

+        .and_then(|s| s.as_string())
+        .unwrap_or_default();
+    serde_json::from_str(&stringval).ok()
+}


This has been updated to move the API changes from persistence to a getter.

finos-cla-bot bot added the cla-present label Sep 12, 2023

texodus added the enhancement Feature requests or improvements label Sep 12, 2023

ada-x64 force-pushed the move-datagrid-styles-to-sidebar branch 5 times, most recently from eb78e37 to dc5e147 Compare September 19, 2023 15:33

ada-x64 added 2 commits September 19, 2023 10:47

Move styles to sidebar

54d6c51

Co-authored-by: Davis Silverman <sinistersnare@users.noreply.github.com> Co-authored-by: Timothy Bess <timbessmail@gmail.com> Co-authored-by: Broch Stilley <brochington@gmail.com> Fix datagrid bugs

Restore column-style-changed event and update tests

f4887cb

add splitby tests

ada-x64 force-pushed the move-datagrid-styles-to-sidebar branch from dc5e147 to f4887cb Compare September 19, 2023 18:59

texodus reviewed Sep 20, 2023

View reviewed changes

ada-x64 commented Sep 20, 2023

View reviewed changes

ada-x64 added 2 commits September 20, 2023 12:46

Move API from persistence to getter. PR changes.

3edf95c

Fix realtime datagrid styling bugs and add tests

9f0b64d

texodus approved these changes Sep 22, 2023

View reviewed changes

texodus merged commit afa7438 into finos:master Sep 22, 2023
12 checks passed

texodus mentioned this pull request Oct 8, 2023

Fix tests #2380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move styles to sidebar #2366

Move styles to sidebar #2366

ada-x64 commented Sep 12, 2023

texodus left a comment

texodus Sep 20, 2023

ada-x64 Sep 20, 2023 •

edited

Loading

texodus Sep 20, 2023

ada-x64 Sep 20, 2023

texodus Sep 20, 2023

texodus Sep 20, 2023

texodus Sep 20, 2023

texodus Sep 20, 2023

texodus Sep 20, 2023

texodus Sep 20, 2023

ada-x64 Sep 20, 2023

texodus Sep 20, 2023

ada-x64 Sep 20, 2023

ada-x64 Sep 20, 2023

Move styles to sidebar #2366

Move styles to sidebar #2366

Conversation

ada-x64 commented Sep 12, 2023

texodus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ada-x64 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ada-x64 Sep 20, 2023 •

edited

Loading