report(viewer): only disable save-gist button on successful save #16618
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
only disable the button after a confirmed successful save, allowing retry on failure. fixes GoogleChrome#14947
niravzxv
force-pushed
the
fix/save-gist-only-on-success
branch
from
4fd7c0e to
769d9c2
Compare
only disable the button after a confirmed successful save, allowing retry on failure. fixes GoogleChrome#14947
Previously, the "Save as Gist" button was disabled immediately after any save attempt, even if the gist creation failed (for example, if the user was not logged in). This change updates the logic so that the button is disabled only after a confirmed, successful save (when a gist ID is returned). This allows users to retry saving in case of failure. Fixes GoogleChrome#14947
niravzxv
changed the title
niravzxv
changed the title
niravzxv
changed the title
connorjclark
left a comment
connorjclark
left a comment
There was a problem hiding this comment.
Seems to work, thanks!
A couple minor issues to resolve then we can merge.
btw I tested by using http://localhost:7333 rather than http://[::]:7333 - only the former is allowed to work in our Firebase settings.
viewer/app/src/viewer-ui-features.js
Outdated
| const saveGistItem = | ||
| this._dom.find('.lh-tools__dropdown a[data-action="save-gist"]', this._dom.rootEl); | ||
| saveGistItem.setAttribute('disabled', 'true'); | ||
| try { |
There was a problem hiding this comment.
is this try/catch needed? _onSaveJson is already catching errors.
| async _onSaveJson(reportJson) { | ||
| if (window.gtag) { | ||
| window.gtag('event', 'report', {type: 'share'}); | ||
| } |
There was a problem hiding this comment.
please don't make unnecessary / unrelated changes:
- don't remove gtag call here
- don't remove "@ private"
- don't make whitespace changes (the comment above this function are misaligned, and the warning log at L174). maybe your IDE is reformatting stuff?
connorjclark
left a comment
connorjclark
left a comment
There was a problem hiding this comment.
looks good, thanks!
2 participants
fixes #14947
Summary
Testing
yarn serve-viewer).Note : Locally domain not authorized to log in success case need to test.