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

[ENG-1354, 1415] Fixes bottom sidesheet and ReactFlow alignment issues #267

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

vsreekanti
Copy link
Contributor

Describe your changes and why you are making these changes

This PR fixes two bugs around the layout of the ReactFlow canvas and the bottom sidesheet:

  • The fix for ENG-1354 is around calling fitView at the right time. Because we were previously calling fitView on componentDidMount, it didn't actually work on the first load because the nodes and edges were loaded in after the component was mounted. This PR changes the fitView effect so that it also is called when the node/edge positions are changed, which causes the view to be fit correctly (after a small lag) on first load.
  • This PR also fixes ENG-1415 simplifies absolute positioning of the bottom side sheet so that it properly aligns with the ReactFlow viewport (see screenshot below):

(Ignore the miscolored artifacts; that's because my local backend isn't updated.)
image

Related issue number (if any)

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • [N/A] If this is a new feature, I have added unit tests and integration tests.
  • [N/A] I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@vsreekanti vsreekanti added the skip_integration_test Skips required integration test (only documentation/UI changes) label Jul 28, 2022
useEffect(() => {
fitView();
}, []);
}, [dagPositionState]);

useEffect(() => {
// NOTE(vikram): There's a timeout here because there seems to be a
Copy link
Contributor

Choose a reason for hiding this comment

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

timeout still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is still necessary. This has to do with a race condition between the CSS transition for the div and when the fitView call is made.

@@ -151,9 +151,8 @@ export const AqueductSidebar: React.FC<Props> = ({
// open bottom to top
bottomAligned: {
position: 'absolute',
right: bottomSideSheetOffset,
left: MenuSidebarOffset,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super urgent, but we'll need to add this to the EnterpriseMenuSidebar as well. we use different components on enterprise and OSS

@agiron123
Copy link
Contributor

Looks good, remember to also update the version number of @aqueducthq/common here as well.

We can say something like v0.0.11-rc.0 so as not to have too many version numbers.

Here's how the folks at emotion do it:
image

@vsreekanti vsreekanti merged commit a827ce1 into main Jul 29, 2022
@vsreekanti vsreekanti deleted the eng-1354-eng-1415 branch July 29, 2022 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip_integration_test Skips required integration test (only documentation/UI changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants