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 React Mosaic #390

Merged
merged 14 commits into from
May 9, 2022
Merged

Adding React Mosaic #390

merged 14 commits into from
May 9, 2022

Conversation

dvzacharycutler
Copy link

@dvzacharycutler dvzacharycutler commented May 5, 2022

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

Developer Checklist (Definition of Done)

  • Descriptive title for this pull request provided (will be used for release notes later)
  • All acceptance criteria from the issue are met
  • Branch is up-to-date with the branch to be merged with, i.e., develop
  • Code is cleaned up and formatted
  • Code documentation written
  • Unit tests written
  • Tested in Chrome
  • Tested in Firefox
  • Build is successful
  • Summary of changes written
  • Wiki documentation written
  • Assign at least one reviewer
  • Assign at least one assignee
  • Add type label (e.g., bug, enhancement) to this pull request
  • Add next version label (e.g., next-version: minor) to this PR following semver

Summary of changes

  • Removing splitpane from Workbenches and adding react-mosaic. Made some style changes to account for this. Changed the rendering logic a bit in workbenches to account for rendering the MosaicWindow component.

Screenshots

react-mosaic.mp4

@dvzacharycutler dvzacharycutler marked this pull request as draft May 5, 2022 15:19
@dvzacharycutler dvzacharycutler marked this pull request as ready for review May 6, 2022 09:59
@dvzacharycutler dvzacharycutler added the type: refactor Refactor the current implementation label May 6, 2022
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 refactoring/feature. The windowing works now much smoother and also looks cleaner from the code.

Please see my comments and improvements below.

Additionally, I noticed that the dragging of the COSMIC view is not fixed using the pointer-events: none. Could you please have a look into this again?

ordino-react-mosaic-dragging

P.S. Ignore the empty ranking which is caused by the lineupengine release v2.4.1.

package.json Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Show resolved Hide resolved
const path = getPathToCorner(mosaicState, Corner.TOP_RIGHT);
const parent = getNodeAtPath(mosaicState, dropRight(path)) as MosaicParent<string>;
const destination = getNodeAtPath(mosaicState, path) as MosaicNode<string>;
const direction: MosaicDirection = parent ? getOtherDirection(parent.direction) : 'row';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const direction: MosaicDirection = parent ? getOtherDirection(parent.direction) : 'row';
const direction: MosaicDirection = parent ? getOtherDirection(parent.direction) : 'row';
const newViewId: string = views[views.length - 1].uniqueId; // assumes that the new view is appended to the array

Maybe extract to a variable and add a comment that new views are appended.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, done. Should also mention here the way this works will probably not quite hold when we get provenance in, since it relies on views being added one at a time and in order. But can address that when the need arises?

Copy link
Member

Choose a reason for hiding this comment

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

Good question! I think, we don't need a todo here, as we need to test everything again once the provenance is in place. Maybe you give @kirangadhave a heads up.

src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchViews.tsx Outdated Show resolved Hide resolved
src/app/workbench/WorkbenchView.tsx Outdated Show resolved Hide resolved
@dvzacharycutler
Copy link
Author

Also made a new ticket for improving the performance of the resizing with vis https://github.com/datavisyn/reprovisyn/issues/373. Currently it is quite slow, and if the dataset is large its a large hindrance

@thinkh
Copy link
Member

thinkh commented May 9, 2022

@dvzacharycutler Thanks for your changes and for creating the follow-up issue. There are two open points before we are ready to merge this PR.

Refactor CSS imports

Please refactor the CSS imports as mentioned above.

Add pointer-events: none

Additionally, I noticed that the dragging of the COSMIC view is not fixed using the pointer-events: none. Could you please have a look into this again?

ordino-react-mosaic-dragging

Did you address this already? I couldn't find any SCSS-related changes.

@dvzacharycutler
Copy link
Author

Yes, I addressed the pointer events issue, the problem wasnt scss but me not realizing what the onDragEnd does. Instead set the flag for dragging in the MosaicWindow onChange, and turned it off in onRelease. Will fix the imports now

@thinkh
Copy link
Member

thinkh commented May 9, 2022

Yes, I addressed the pointer events issue, the problem wasnt scss but me not realizing what the onDragEnd does. Instead set the flag for dragging in the MosaicWindow onChange, and turned it off in onRelease.

Ok, thanks for the explanation.

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.

I've tested the PR again and it works well. Dragging is working fluently and without problems.

I could reproduce a bug with LineUp when a second view is dragged to the left. I documented the bug in the follow-up issue https://github.com/datavisyn/reprovisyn/issues/375.

Beside this everything else looks good and is ready to be merged. Thanks for your implementation, @dvzacharycutler!

@thinkh thinkh merged commit 8b4d6ca into ordino-2.0 May 9, 2022
@thinkh thinkh deleted the zc/react-mosaic branch May 9, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactor Refactor the current implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants