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

Performance: More careful polling of graphql data #1125

Merged
merged 14 commits into from
Dec 9, 2021
Merged

Performance: More careful polling of graphql data #1125

merged 14 commits into from
Dec 9, 2021

Conversation

stackoverfloweth
Copy link
Contributor

PR Checklist:

  • add a short description of what's changed to the top of the CHANGELOG.md
  • add/update tests (or don't, for reasons explained below)

Describe this PR

@stackoverfloweth stackoverfloweth marked this pull request as ready for review November 12, 2021 19:57
@stackoverfloweth stackoverfloweth changed the title Bugfix: More careful polling of graphql data Performance: More careful polling of graphql data Nov 18, 2021
@stackoverfloweth stackoverfloweth self-assigned this Nov 18, 2021
zhen0
zhen0 previously requested changes Nov 19, 2021
View reviewed changes
Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

This is looking good. One small nitpick from me - when I refresh the page I get the "no flows" banner until all my flows are loaded - this doesn't happen in master/staging. Can we add a check for loading before we show that banner? Thanks!

@stackoverfloweth stackoverfloweth dismissed zhen0’s stale review November 30, 2021 15:31

concerns have been addressed

zhen0
zhen0 previously approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@zhen0 zhen0 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @stackoverfloweth

@zhen0
Copy link
Member

zhen0 commented Dec 1, 2021

@cicdw - since this one touches graphQL it needs your approval before we can merge.

@stackoverfloweth
Copy link
Contributor Author

@cicdw let me know if you'd like me to walk through the query changes, there are a lot here that is unrelated to the graphQL change

@zhen0 zhen0 merged commit 74e0580 into master Dec 9, 2021
@zhen0 zhen0 deleted the ui-performance/query-subscriptions branch December 9, 2021 14:05
@zhen0 zhen0 mentioned this pull request Dec 14, 2021
Merged
This was referenced Dec 14, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhen0 zhen0 zhen0 approved these changes

@cicdw cicdw cicdw approved these changes

@jlowin jlowin Awaiting requested review from jlowin jlowin is a code owner

@znicholasbrown znicholasbrown Awaiting requested review from znicholasbrown znicholasbrown is a code owner

Assignees

@stackoverfloweth stackoverfloweth

Labels
Chris approval needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stackoverfloweth @zhen0 @cicdw