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

[KED-2446] Add new 'sizewarning' flag #376

Merged
merged 7 commits into from
Feb 22, 2021

Conversation

richardwestenra
Copy link
Contributor

Description

  1. Add a new 'sizewarning' flag for the large graph warning. This flag is enabled by default, but it's in place so that users can disable it if necessary, if the warning causes them problems. This will also allow us to add a "Don't show this message again" link/button, if we like.
  2. Add new tests for the getTriggerLargeGraphWarning selector: Add a test for the flag case, but also add new tests to handle the ignoreLargeWarning and graphHasNodes cases too, which were missed out of [KED-2280] Display size warnings on large graphs #361.
  3. Update getTriggerLargeGraphWarning JSDoc comment, to add a bit more detail explaining under what conditions it should return true (i.e. show the warning).

Development notes

For the graphHasNodes test, I needed the state to have a sufficiently large dataset to cross the threshold and trigger the warning, but for the layout to have already been calculated so that the graph object has been updated and populated. In order to achieve this, I prepared the state with a large dataset, then ran it through a reducer with 3 actions: Toggling on a node-type filter to bring it under the threshold, updating the graph state, then toggling the filter back off again to bring the graph back above the threshold.

QA notes

If you set ?sizewarning=0 in the URL with a very large pipeline, the large graph warning shouldn't display.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

Add a test for the flag case, but also add  new tests to handle the ignoreLargeWarning and graphHasNodes cases too.
Add a bit more detail to explain under what conditions it should return true (i.e. show the warning)
This is necessary in order to ensure that this test has a sufficiently large graph to exceed the threshold, but that it still returns false anyway
'toBe' makes more sense for Booleans
Copy link
Contributor

@studioswong studioswong 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 setting this PR and adding the tests @richardwestenra, looks great! I just have one confusion about the last test you added in, which I don't think is a scenerio that would exist and having that test in would be rather confusing on the expected behavior for this feature.

Regardless, great stuff!

Copy link
Contributor

@studioswong studioswong 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 clarification - all clear about the test case.

I've left a suggested change for the wording of the test case to make the expected behaviour more apparent.

@richardwestenra richardwestenra merged commit 505169d into main Feb 22, 2021
@richardwestenra richardwestenra deleted the feature/size-warning-flag branch February 22, 2021 13:49
@richardwestenra richardwestenra mentioned this pull request Mar 2, 2021
1 task
richardwestenra added a commit that referenced this pull request Mar 2, 2021
# Release 3.10.0

## Major features and improvements

- Display a prompt before rendering very large graphs (#361, #376, #377, #381)

## Bug fixes and other changes

- Clean up SCSS tech debt (#380)
- Add Container component to simplify app/lib entrypoint (#379)
- Add stylelint 'rule-empty-line-before': 'always' (#378)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants