Fix "Handling the return value" example#44627
Open
internet-account2 wants to merge 2 commits into
Open
Conversation
- Code description was inaccurate - Falsely stated that "Esc" doesn't trigger close event - "Confirm" button having a value that is never used is confusing - Code didn't check for the initial "" returnValue, which seems like an oversight to me
github-actions
Bot
added
Content:HTML
Contributor
|
Preview URLs (1 page) (comment last updated: 2026-07-03 13:49:45) |
Contributor
Author
|
Oh, maybe it should be mentioned that on pressing Esc a |
should make it clear that it's possible to separately handle the cancel event?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Fixes inaccuracies and oversights in the example description and code.
Maybe the code could say
<option value>Choose…</option>instead of<option value="">Choose…</option>?And maybe instead of
favDialog.returnValue === ""the truthiness could be checked instead.But probably fine as it is?
Of course it could also check for
(favDialog.returnValue === "default" || favDialog.returnValue === "")instead and leave the<option value="default">Choose…</option>untouched, but in any way it probably should check for the initial "" value in some way IMO.Motivation
Descriptions that don't match the code are confusing.
Code with additional bits that have no influence on the behavior even though they look like they might is confusing.
Code that doesn't handle edge cases makes it easy to forget that they exist, leading to potential bugs along the road.
Clearing those things up will hopefully be beneficial to the average reader.
Additional details
Related issues and pull requests