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.

Sign up for GitHub

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

Jump to bottom

Graph view hides resources with default zoom #2390

Merged
merged 7 commits into from
Jul 1, 2022
Merged

Graph view hides resources with default zoom #2390

merged 7 commits into from
Jul 1, 2022

Conversation

opudrovs
Copy link
Contributor

@opudrovs opudrovs commented Jul 1, 2022 •
edited

Closes #2312

  • Fixed the initial zoom level and position of the graph.
  • Improved graph positioning performance thanks to using d3 translate instead of the SVG viewBox attribute.
  • Reduced graph zoom level (added custom mapping from slider scale to graph zoom percent).
  • Added graph utility functions (for converting and mapping zoom-related values) to utils and added tests for them.
  • Fixed version keys in the footer (because it produced a warning when running tests, I had wrapped the version elements with React fragments but had forgotten to move the keys to fragments).
  • Reworked D3Graph props and types and zoom options.
  • Improved graph positioning in general. Hopefully, other graph-related issues will be easier to handle in the future after this rework and refactoring.
  • Added a loading text message to display while the graph is rendering the nodes (until it is possible to the size and position of the root node).
  • The issue Graph can not zoom and maintain drag status #1225 will be handled later in a separate PR because the current branch already introduces many changes.

@opudrovs opudrovs added bug Something isn't working team/denim labels Jul 1, 2022
@opudrovs opudrovs force-pushed the 2312-graph-hides-resources-with-default-zoom branch from 0a14ba0 to acab07b Compare July 1, 2022 09:57
@opudrovs
Fix initial graph zoom.
fa46cc4 
Add a loading text message to display while the graph is rendering.
@opudrovs
Fix graph flickering on zoom.
edbaa82 
Rework D3Graph props and types.
@opudrovs
Refactor zoom options.
997ca5b
@opudrovs opudrovs force-pushed the 2312-graph-hides-resources-with-default-zoom branch from acab07b to f85f642 Compare July 1, 2022 10:25
@opudrovs
Reduce zoom level of the graph and add mapping scale to zoom percent …
4c64dc4 
…and back.

Move graph utility functions to utils and add tests for them.

Fix version keys in the footer.
@opudrovs opudrovs force-pushed the 2312-graph-hides-resources-with-default-zoom branch from 605fe60 to 4c64dc4 Compare July 1, 2022 11:02
@opudrovs opudrovs marked this pull request as ready for review July 1, 2022 12:48
@opudrovs opudrovs requested review from ozamosi and joshri July 1, 2022 12:48
@opudrovs
Copy link
Contributor Author

opudrovs commented Jul 1, 2022

@ozamosi @joshri this branch not ready for testing yet, just found a minor issue with loading text when I finally managed to start my Docker. I will let you know as soon as I am done with the final testing.

@opudrovs opudrovs force-pushed the 2312-graph-hides-resources-with-default-zoom branch from aca653d to 28acf2d Compare July 1, 2022 13:17
@opudrovs
Copy link
Contributor Author

opudrovs commented Jul 1, 2022 •
edited

@ozamosi @joshri checked it, it's ready for testing.

It was a rare positive issue: now, after the latest improvements, the graph loads much faster (comparing to the previous initial zoom fix which caused flickering), so there was no loading text displayed and I had thought smth. was wrong. Now there is just no delay before the root node is rendered, but I still kept the loading text for systems with lower performance. Please let me know if you still see the loading text.

Please use the graphs from the repo in the issue #2312 description for testing, those graphs are really wide.

And please check the font size at the initial and final zoom is not too small.

ozamosi
ozamosi approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@ozamosi ozamosi left a comment

Choose a reason for hiding this comment

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

  • A small graph shows up properly.

  • Looking at a big graph centers on the source.

  • The default size gives you a reasonable text size

  • I can zoom out pretty much as far as I can reasonably distinguish letters.

  • When I zoom on a centered graph, the graph indeed moves sideways. This is a documented limitation and tracked separately.

ui/lib/__tests__/utils.test.ts Outdated
it("calculates zoom ratio", () => {
expect(
Math.abs(calculateZoomRatio(0) - 0.013333333333333334)
).toBeLessThanOrEqual(floatDiff);
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to express this is using https://jestjs.io/docs/expect#tobeclosetonumber-numdigits - it reads well to me because it lets you put the actual and the expected values on opposite "sides" - but this works too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good to know! Will refactor the tests with it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored the tests with toBeCloseTo.

joshri
joshri approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@joshri joshri left a comment

Choose a reason for hiding this comment

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

👏 genius 👏

@opudrovs
Copy link
Contributor Author

opudrovs commented Jul 1, 2022

@joshri thanks, that's more like survival skills. 😅

@opudrovs opudrovs merged commit d5a49c4 into main Jul 1, 2022
@opudrovs opudrovs deleted the 2312-graph-hides-resources-with-default-zoom branch July 1, 2022 16:49
This was referenced Jul 26, 2022
Merged
Merged
@weave-gitops-bot weave-gitops-bot mentioned this pull request Aug 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozamosi ozamosi ozamosi approved these changes

@joshri joshri joshri approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Graph view hides resources with default zoom
3 participants
@opudrovs @ozamosi @joshri