-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix(html): mirror query clearing in URL #33689
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Current behavior is by design - look at what you've done to the history (try typing something long and then going back). I don't think this issue is work fixing - we don't want typing (and clearing) to affect the history. They are free to hit Enter to reset the url. Your semantics would not need the form btw, surprised you did non delete it! |
Good catch with the history, didn't think about that. Agree that this isn't the most valuable fix to ship, but i'll still have a look if we can update the query on clicking the x without polluting history. |
setFilterText(e.target.value); | ||
const filterText = e.target.value; | ||
if (filterText) | ||
setFilterText(filterText); |
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.
Updating the value state synchronously and unconditionally would bring me great comfort. My React could be better, but isn't it a general requirement?
if (filterText) | ||
setFilterText(filterText); | ||
else | ||
navigate('#'); |
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.
So if I'm mostly using keyboard and do Select all + Backspace or press and hold Backspace you want me to have an unusual history treatment? Not sure it is within the accepted UX guidelines. Why does Input have no "clearedUsingMouse" event for that use case?
I think "Submit", "Enter" or "Click" should navigate, while typing in the keyboard should not navigate. If input's clear button was accessible and had a dedicated handler, we could consider it a trigger event. But as it is now, clear button is just a shortcut for typing Backspace a bunch of times, or select all + backspace. That can't navigate. |
Test results for "tests 1"1 flaky37140 passed, 650 skipped Merge workflow run. |
Closes #33681