-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add Missing URL state to Navigation Link Block #28861
Merged
Merged
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
bfb6f10
Prototyping a new Link Nav Block Interaction
410eb6d
Use the existing LinkControl for Link Block
8e25425
Clean up implementation
b03a5b9
Add border to popover
8391495
Polish
jasmussen 70df9fd
Get the sizing to work.
jasmussen 6a003bd
Color the separator.
jasmussen 7bee744
Customize Missing URL text based on variation
2f0c2c2
Trigger URL Input on Keyboard Focus
8711c5d
Update missing labels
c2344ac
Add Highlighting to Inactive State
07c8671
Polish border and spacing.
jasmussen 1ffb93b
Add keyboard shortcuts for displaying popover
93daed3
Prettier
06ca25e
Fix Keyboard and Click Focus
8ab3153
Add pointer cursor for missing state
3ae7de1
Remove isAlternate
9f6cf37
Simplify Promise cancel
0509b2a
Add translator notes
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
Found this during debugging, if you quickly toggle the
URLInput
before the fetch returns, it will attempt to callsetState()
on the unmounted component. This cancels thesuggestionsRequest
Promise when it unmounts to avoid the issue.I can break it out to a separate PR if needed, the rest of the code doesn't trigger this anymore.
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.
Probably okay to sneak in, maybe just leave a quick note in the summary. eg "also fixes ..."
Minor: We can remove the if check using optional chaining.
See also: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#stacking_the_optional_chaining_operator