-
Notifications
You must be signed in to change notification settings - Fork 893
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
Adds toast ID to toast api #3752
Adds toast ID to toast api #3752
Conversation
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #3752 +/- ##
==========================================
+ Coverage 66.43% 66.45% +0.01%
==========================================
Files 3209 3208 -1
Lines 61741 61752 +11
Branches 9537 9539 +2
==========================================
+ Hits 41020 41035 +15
+ Misses 18431 18429 -2
+ Partials 2290 2288 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -42,12 +42,10 @@ import { I18nStart } from '../../i18n'; | |||
/** | |||
* Allowed fields for {@link ToastInput}. | |||
* | |||
* @remarks | |||
* `id` cannot be specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know the historical reason why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the git history but as far as I can tell, there isn't a good reason. That's why I kept the default behavior as is. Only if you explicitly pass an id does the behavior change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing, my understanding is that it was just intended to be an internal property, that simply auto-increments with each new toast. Ashwin's change here changes the purpose and usage of the property a bit (for example, passing string ids instead of a number), but not in a way that affects the historical usage. So I don't think it was intended as "specifying id is prohibited" and instead meant "specifying id is not necessary".
@ashwin-pc I tried re-running the cypress tests, but you may want to take a look at why there are failed tests there. |
The branch was behind main by quite a bit. They should be passing now |
return existingToast; | ||
} | ||
} | ||
|
||
const toast: Toast = { | ||
id: String(this.idCounter++), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's somewhat surprising that we still increment the idCounter
, but throw away the value. On the one hand, it means that the counter really does match the total number, but on the other, there are now some idCounter values that don't exist in the map of toast IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont still increment, if an existing toast is found, we exit this function when we return the existing toast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'm talking about the case where an id
is provided, but there's no existing matching toast. In that case we increment, but don't use the incremented value.
src/plugins/vis_builder/public/application/utils/state_management/store.ts
Show resolved
Hide resolved
Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>
* Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that * Update changelog * Update src/core/public/notifications/toasts/toasts_api.tsx Issue Resolved: #2643 Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Sean Neumann <1413295+seanneumann@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 14bde2b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* Adds toast ID to toast api (#3752) * Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that * Update changelog * Update src/core/public/notifications/toasts/toasts_api.tsx Issue Resolved: #2643 Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> --------- Signed-off-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Sean Neumann <1413295+seanneumann@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 14bde2b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md * add changelog Signed-off-by: Josh Romero <rmerqg@amazon.com> --------- Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Anan Zhuang <ananzh@amazon.com>
Description
Currently sending the same toast multiple times results in multiple toasts being rendered on the screen. This change allows the toast api to additionally accept an id parameter that
Before:
Now:
Issues Resolved
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr