Skip to content
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

Adding hexbin to vis #790

Merged
merged 35 commits into from
Feb 3, 2023
Merged

Adding hexbin to vis #790

merged 35 commits into from
Feb 3, 2023

Conversation

dvzacharycutler
Copy link
Contributor

@dvzacharycutler dvzacharycutler commented Sep 21, 2022

Closes https://github.com/datavisyn/reprovisyn/issues/253

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

  • Adding back the hexbin plot now that d3 is compatible. Also includes small changes to the selection logic which drastically improves speed for large selections. Also made the vis component import dynamically in the ARankingView, so as not to always import mantine in tdp_core, but importantly i HAVE NOT tested this yet, as reprovisyn doesn't actually use the component. Needs to be tested in an application that uses the Vis component via the ARankingView.

Screenshots

newHexin.webm

Additional notes for the reviewer(s)

  • TODO:: Test the Vis component via ARankingView, as mentioned in summary of changes
    Thanks for creating this pull request 🤗

@dvzacharycutler dvzacharycutler changed the title Zc/mantine hexbin Adding hexbin to vis Sep 21, 2022
@dvzacharycutler dvzacharycutler marked this pull request as ready for review September 21, 2022 12:57
@dvzacharycutler dvzacharycutler requested a review from a team as a code owner September 21, 2022 12:57
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome new visualization. It looks already very good. Please see my comments below.

  • The mouse while should work in

If no numerical columns are selected the message currently takes the full height of the vies view (should be only the height of the text).

image

  • Hexbin plot (sentence case)
  • Invalid settings (sentence case)
  • Convert invalid settings card to Mantine info alert (currently bootstrap)
    • Also for all other cards and visualizations
  • Horizontally center the info alert
  • Hexbin options (sentence case)
    image
  • Add tooltips to brush option buttons
    image
  • Enable zoom with mouse wheel in brush mode (currently, it only works in pan mode)
  • Make list of categories in legend scrollable
    image

src/vis/sidebar/HexbinOptionSelect.tsx Outdated Show resolved Hide resolved
src/vis/hexbin/HexbinVisSidebar.tsx Outdated Show resolved Hide resolved
src/vis/hexbin/Hexplot.tsx Outdated Show resolved Hide resolved
src/vis/hexbin/Hexplot.tsx Outdated Show resolved Hide resolved
src/vis/hexbin/Hexplot.tsx Outdated Show resolved Hide resolved
src/vis/hexbin/PieChart.tsx Show resolved Hide resolved
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvzacharycutler Thanks for your bug fixes. It works now much better. I still found a few inconsistencies that should be removed before merging this PR.

Steps to reproduce

  1. Select hexbin plot
  2. Remove all columns

Observed behavior

The message shows Scatterplot.

grafik

Expected behavior

  1. The message should mention the Hexbin plot when this plot is selected
  2. Consistent spelling: Scatter plot vs. Scatterplot

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvzacharycutler I tested this branch with Ordino public in a local workspace and found a couple of problems. The problems might be related to #786 and not specifically to this PR. You can decide where to fix it.

Here an overview of the interaction with the general visualization of this branch:

ordino-hexbin.webm

Scatter plot: Opacity does not have an effect

Sometimes the opacity slider does not have any effect.

ordino-scatter-opacity-0

ordino-scatter-opacity-1

As you can see in the video above, it worked somehow after playing around and switching visualization. But it does not work when I opened it the first time (see screenshots).

Scatter plot: Legend overlaying visualization

The legend is overlaying the visualization and makes it impossible to use. Could be a flex problem in Ordino?

ordino-scatter-overplotting

Hexbin plot: Wrong options panel position

When switching to hexbin plot the options panel is positioned absolute to the window and not the visualization container. (This does not happen for the other visualizations.) After closing and opening the panel again it is positioned correctly.

Furthermore the hexbin plot is empty which should be the case because the scatter plot shows data.

ordino-broken-hexbin

Various plots: scroll position and options panel

For all plot types the visualization is positioned far to the right which requires to scroll to the right. Then the options panel is positioned somewhere in the center.

ordino-stripplot-scrolling

ordino-stripplot-scrolling-options-open

@thinkh
Copy link
Member

thinkh commented Nov 7, 2022

In addition, I saw now that the options panel is also overlaying the Ordino start menu. The options panel should be ordered behind the start menu.

image

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the last fixes. Looks good to me.

@thinkh thinkh merged commit de26819 into zc/mantineVis Feb 3, 2023
@thinkh thinkh deleted the zc/mantineHexbin branch February 3, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants