Close notification UI if no unapproved confirmations #8358
Merged
+63
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rekmarks
requested review from
danjm,
Gudahtt,
jennypollack and
whymarrh
as code owners
Gudahtt
requested changes
Apr 18, 2020
Gudahtt
requested changes
Apr 20, 2020
There was a problem hiding this comment.
There are two things not considered in the current check: connect requests, and suggested tokens. Both of these can already be checked by existing props (firstPermissionRequestId and suggestedTokens), so it should be simple enough to add those to the condition.
rekmarks
force-pushed
the
close-notification
branch
from
b3e258d to
a8e8287
Compare
Builds ready [79b96ce]
|
yqrashawn
pushed a commit
to Conflux-Chain/conflux-portal
that referenced
this pull request
Apr 27, 2020
* close notification UI if no pending confirmations * change benchmark page to 'home'
yqrashawn
pushed a commit
to Conflux-Chain/conflux-portal
that referenced
this pull request
Apr 27, 2020
* close notification UI if no pending confirmations * change benchmark page to 'home'
yqrashawn
pushed a commit
to Conflux-Chain/conflux-portal
that referenced
this pull request
Apr 27, 2020
* close notification UI if no pending confirmations * change benchmark page to 'home'
yqrashawn
pushed a commit
to Conflux-Chain/conflux-portal
that referenced
this pull request
Apr 27, 2020
* close notification UI if no pending confirmations * change benchmark page to 'home'
Gudahtt
added a commit
that referenced
this pull request
Apr 28, 2020
The page tested by the benchmark was changed from `notification` to `home` in #8358, but the announce script was still expecting the `notification` page to be in the results. It does collect results for all pages, but the `notification` page was hard-coded to be used for the benchmark summary. The announce script now correctly looks for the `home` page results for the benchmark summary. Variable names have been updated to make it more clear what's going on here as well.
Gudahtt
added a commit
that referenced
this pull request
Apr 28, 2020
The page tested by the benchmark was changed from `notification` to `home` in #8358, but the announce script was still expecting the `notification` page to be in the results. It does collect results for all pages, but the `notification` page was hard-coded to be used for the benchmark summary. The announce script now correctly looks for the `home` page results for the benchmark summary. Variable names have been updated to make it more clear what's going on here as well.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8348 (well enough)
Attempts to resolve #8348 by ensuring that the notification UI is closed if there are no unapproved confirmations to approve.
Unapproved confirmations include (exhaustively):