fix: vis is rendered with an invalid config #172

oltionchampari · 2024-02-07T08:03:59Z

Developer Checklist (Definition of Done)

Issue

All acceptance criteria from the issue are met
Tested in latest Chrome/Firefox

UI/UX/Vis

Requires UI/UX/Vis review
- Reviewer(s) are notified (tag assignees)
- Review has occurred (link to notes)
- Feedback is included in this PR
- Reviewer(s) approve of concept and design

Code

Branch is up-to-date with the branch to be merged with, i.e., develop
Code is cleaned up and formatted
Unit tests are written (frontend/backend if applicable)
Integration tests are written (if applicable)

PR

Descriptive title for this pull request is provided (will be used for release notes later)
Reviewer and assignees are defined
Add type label (e.g., bug, feature) to this pull request
Add release label (e.g., release: minor) to this PR following semver
The PR is connected to the corresponding issue (via Closes #...)
Summary of changes is written

Summary of changes

We have the issue that we are providing vis an externalConfig that does not include all possible defaults

  const config = {
    type: 'Heatmap plot',
    catColumnsSelected: [
      { id: 'Column 1', name: 'Column 1' },
      { id: 'Column 2', name: 'Column 2' },
    ],
  };

This is the different values the vis was being rendered with (causes errors) when inputing the above config

MainApp.tsx: Parent {type: 'Heatmap plot', catColumnsSelected: Array(2)}
MainApp.tsx:40 Parent null
MainApp.tsx:40 Parent {type: 'Heatmap plot', catColumnsSelected: Array(2), color: {…}}
MainApp.tsx:40 Parent {type: 'Heatmap plot', color: {…}, catColumnsSelected: Array(2), numColorScaleType: 'Sequential', xSortedBy: 'CAT_ASC', …}

And this with this fix

MainApp.tsx: Parent {type: 'Heatmap plot', catColumnsSelected: Array(2)}
MainApp.tsx:40 Parent {type: 'Heatmap plot', color: {…}, catColumnsSelected: Array(2), numColorScaleType: 'Sequential', xSortedBy: 'CAT_ASC', …}

We could improve the logic and readabilty of this section of code when we switch completely to mantine 7 to use https://mantine.dev/hooks/use-uncontrolled/

Screenshots

before

after

Additional notes for the reviewer(s)

Thanks for creating this pull request 🤗

oltionchampari · 2024-02-08T12:38:41Z

src/vis/heatmap/HeatmapGrid.tsx

-      {status === 'pending' ? (
-        <Loader />
-      ) : !hasAtLeast2CatCols ? (
-        <InvalidCols headerMessage="Invalid settings" bodyMessage="To create a heatmap chart, select at least 2 categorical columns." />
-      ) : (
+      {status === 'pending' && <Loader />}


The Invalid settings message is shown shortly while the state is idle. This change fixes that. The other visualizations account for the idle state already.

Thanks for fixing this issue and the mention.

oltionchampari · 2024-02-08T12:43:02Z

src/vis/heatmap/HeatmapText.tsx

@@ -26,7 +26,7 @@ export function HeatmapText({
  isImmediate: boolean;
 }) {
  const labelSpacing = useMemo(() => {
-    const maxLabelLength = d3.max(yScale.domain().map((m) => m.length));
+    const maxLabelLength = d3.max(yScale.domain().map((m) => m?.length));


Accounts for null categories. They are still shown in the visualization but at least the application does not crash.
I have created an issue to discuss possible better solutions

puehringer · 2024-02-08T13:01:18Z

@oltionchampari regarding your last point of using useUncontrolled when we are using mantine7. develop is currently using mantine7, and mantine6 has the hook as well. So why don't we do it right away? 🤔

oltionchampari · 2024-02-08T13:04:53Z

@oltionchampari regarding your last point of using useUncontrolled when we are using mantine7. develop is currently using mantine7, and mantine6 has the hook as well. So why don't we do it right away? 🤔

I thought it was in 7 and we would need the change soonish in v7.0.3. But since its in v6 already we can go ahead and change it already

puehringer · 2024-02-08T13:08:04Z

@oltionchampari regarding your last point of using useUncontrolled when we are using mantine7. develop is currently using mantine7, and mantine6 has the hook as well. So why don't we do it right away? 🤔

I thought it was in 7 and we would need the change soonish in v7.0.3. But since its in six already we can go ahead and change it already

Ok, now I understand: this PR is intended to be released for a previous visyn_core version, right? But then the develop PR target isn't really correct, as that is latest (Mantine 7 only).

oltionchampari · 2024-02-08T13:09:44Z

@oltionchampari regarding your last point of using useUncontrolled when we are using mantine7. develop is currently using mantine7, and mantine6 has the hook as well. So why don't we do it right away? 🤔

I thought it was in 7 and we would need the change soonish in v7.0.3. But since its in six already we can go ahead and change it already

Ok, now I understand: this PR is intended to be released for a previous visyn_core version, right? But then the develop PR target isn't really correct, as that is latest (Mantine 7 only).

We will cherry pick this commit and merge it to v7.0.3 as far as i understand.

puehringer · 2024-02-08T13:11:42Z

@oltionchampari regarding your last point of using useUncontrolled when we are using mantine7. develop is currently using mantine7, and mantine6 has the hook as well. So why don't we do it right away? 🤔

I thought it was in 7 and we would need the change soonish in v7.0.3. But since its in six already we can go ahead and change it already

Ok, now I understand: this PR is intended to be released for a previous visyn_core version, right? But then the develop PR target isn't really correct, as that is latest (Mantine 7 only).

We will cherry pick this commit and merge it to v7.0.3 as far as i understand.

Yeah, that makes sense. I would still like to see the useUncontrolled in case you have time to add it. But thanks for the PR!

oltionchampari · 2024-02-09T09:05:16Z

@puehringer Please take another look when possible.
Note the new screencasts of interactions before and current.

One notable difference now is that once the EagerVis receives an external config we merge the defaults to it but we do not update the config of the parent, that will occur once the user changes a config in the sidebar. This saves a couple of updates.
Note the short flickering of the type input is gone in this branch

thinkh

Thanks for addressing the suggestion and updating the example. It looks good to me.

@puehringer please take a final look. Thanks.

**Issue** - [ ] All acceptance criteria from the issue are met - [x] Tested in latest Chrome/Firefox **UI/UX/Vis** - [ ] Requires UI/UX/Vis review - [ ] Reviewer(s) are notified (_tag assignees_) - [ ] Review has occurred (_link to notes_) - [ ] Feedback is included in this PR - [ ] Reviewer(s) approve of concept and design **Code** - [x] Branch is up-to-date with the branch to be merged with, i.e., develop - [x] Code is cleaned up and formatted - [ ] Unit tests are written (frontend/backend if applicable) - [ ] Integration tests are written (if applicable) **PR** - [x] Descriptive title for this pull request is provided (will be used for release notes later) - [x] Reviewer and assignees are defined - [x] Add type label (e.g., *bug*, *feature*) to this pull request - [x] Add release label (e.g., `release: minor`) to this PR following [semver](https://semver.org/) - [x] The PR is connected to the corresponding issue (via `Closes #...`) - [x] [Summary of changes](#summary-of-changes) is written We have the issue that we are providing vis an externalConfig that does not include all possible defaults ```typescript const config = { type: 'Heatmap plot', catColumnsSelected: [ { id: 'Column 1', name: 'Column 1' }, { id: 'Column 2', name: 'Column 2' }, ], }; ``` This is the different values the vis was being rendered with (causes errors) when inputing the above config ``` MainApp.tsx: Parent {type: 'Heatmap plot', catColumnsSelected: Array(2)} MainApp.tsx:40 Parent null MainApp.tsx:40 Parent {type: 'Heatmap plot', catColumnsSelected: Array(2), color: {…}} MainApp.tsx:40 Parent {type: 'Heatmap plot', color: {…}, catColumnsSelected: Array(2), numColorScaleType: 'Sequential', xSortedBy: 'CAT_ASC', …} ``` And this with this fix ``` MainApp.tsx: Parent {type: 'Heatmap plot', catColumnsSelected: Array(2)} MainApp.tsx:40 Parent {type: 'Heatmap plot', color: {…}, catColumnsSelected: Array(2), numColorScaleType: 'Sequential', xSortedBy: 'CAT_ASC', …} ``` We could improve the logic and readabilty of this section of code when we switch completely to mantine 7 to use https://mantine.dev/hooks/use-uncontrolled/ before ![gr-original](https://github.com/datavisyn/visyn_core/assets/51322092/ea036357-6411-4592-aaf2-a1b469ce2d3e) after ![gr-new](https://github.com/datavisyn/visyn_core/assets/51322092/a9e2aac0-b55a-4395-a25b-7eb0e908fdd0) - Thanks for creating this pull request 🤗 (cherry picked from commit 30da502)

## What's changed * fix: cypress tests (#159) * fix: vis is rendered with an invalid config (#172) * feat: add POSTGRES_PORT to get_default_postgres_url (#181) * chore(deps): bump cachetools from 5.3.1 to 5.3.2 (#147)

oltionchampari added 4 commits February 7, 2024 09:01

fix: merge visConfig defaults before painting

72c8b8b

fix: null category in heatmap

1857073

fix: heatmap invalid settings

92ec4fb

fix: try a simpler solution

1d9b01d

oltionchampari commented Feb 8, 2024

View reviewed changes

oltionchampari changed the title ~~fix: vis is rendered without a valid config~~ fix: vis is rendered with an invalid config Feb 8, 2024

oltionchampari requested a review from thinkh February 8, 2024 12:47

oltionchampari self-assigned this Feb 8, 2024

oltionchampari requested a review from puehringer February 8, 2024 12:47

oltionchampari added type: bug Something isn't working release: patch PR merge results in a new patch version labels Feb 8, 2024

Merge branch 'develop' into och/vis-initialization

0950d3e

oltionchampari marked this pull request as ready for review February 8, 2024 12:58

fix: use style instead of sx

426854c

refactor: use useUncontrolled hook in EagerVis

7a6266e

thinkh approved these changes Feb 12, 2024

View reviewed changes

puehringer merged commit 30da502 into develop Feb 16, 2024
4 checks passed

puehringer deleted the och/vis-initialization branch February 16, 2024 15:13

dvvanessastoiber mentioned this pull request Feb 21, 2024

Release 8.0.1 #190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: vis is rendered with an invalid config #172

fix: vis is rendered with an invalid config #172

oltionchampari commented Feb 7, 2024 •

edited

Loading

oltionchampari Feb 8, 2024 •

edited

Loading

thinkh Feb 12, 2024

oltionchampari Feb 8, 2024 •

edited

Loading

puehringer commented Feb 8, 2024

oltionchampari commented Feb 8, 2024 •

edited

Loading

puehringer commented Feb 8, 2024

oltionchampari commented Feb 8, 2024

puehringer commented Feb 8, 2024

oltionchampari commented Feb 9, 2024 •

edited

Loading

thinkh left a comment

fix: vis is rendered with an invalid config #172

fix: vis is rendered with an invalid config #172

Conversation

oltionchampari commented Feb 7, 2024 • edited Loading

Developer Checklist (Definition of Done)

Summary of changes

Screenshots

Additional notes for the reviewer(s)

oltionchampari Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

thinkh Feb 12, 2024

Choose a reason for hiding this comment

oltionchampari Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

puehringer commented Feb 8, 2024

oltionchampari commented Feb 8, 2024 • edited Loading

puehringer commented Feb 8, 2024

oltionchampari commented Feb 8, 2024

puehringer commented Feb 8, 2024

oltionchampari commented Feb 9, 2024 • edited Loading

thinkh left a comment

Choose a reason for hiding this comment

oltionchampari commented Feb 7, 2024 •

edited

Loading

oltionchampari Feb 8, 2024 •

edited

Loading

oltionchampari Feb 8, 2024 •

edited

Loading

oltionchampari commented Feb 8, 2024 •

edited

Loading

oltionchampari commented Feb 9, 2024 •

edited

Loading