LibraryPanelSearch: Refactor and fix hyphen issue #55314
Conversation
kaydelaney
added
area/search
area/frontend
add to changelog
area/frontend/library-panels
oss-user-essentials
backport v9.1.x
kaydelaney
requested review from
a team,
ashharrison90,
academo,
codeincarnate and
oscarkilhed
and removed request for
a team
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/33845 |
There was a problem hiding this comment.
Thank you so much for the refactor @kaydelaney, it looks great!
While reproducing the issue, I cannot do it I cannot reproduce it in play. Am I doing something wrong? I tried also adding a panel and I got the same result.
|
|
||
| return ( | ||
| <div className={styles.container}> | ||
| {showSort && <SortPicker value={sortDirection} onChange={onSortChange} filter={['alpha-asc', 'alpha-desc']} />} |
There was a problem hiding this comment.
Should we extract the filter strings to an enum here? WDYT?
There was a problem hiding this comment.
It is indeed not otherwise obvious that those two are part of the options SortPicker has after receiving its response from the api /api/search/sorting.
public/app/features/library-panels/components/LibraryPanelsSearch/LibraryPanelsSearch.tsx
Outdated
Show resolved
Hide resolved
@ivanortegaalba Me neither, but I do reproduce it locally and it is fixed on this PR. I also don't know why it's okay in Play 👾. |
|
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34231 |
| } | ||
|
|
||
| export const FilterInput = React.forwardRef<HTMLInputElement, Props>( | ||
| ({ value, width, onChange, ...restProps }, ref) => { | ||
| ({ value, width, onChange, escapeRegex = true, ...restProps }, ref) => { |
There was a problem hiding this comment.
Because FilterInput is in grafana-ui should we make the naming of the prop more clear about its purpose in general, like escapeSpecialChars?
escapeRegex makes me wonder which regex, what is the regex if not the one inside FilterInput and what does escaping it mean if it is?
There was a problem hiding this comment.
Tested locally, and it works perfectly fine! I can now use special characters as part of the search. The original issue was only in main but not in play, for the records ;P
Thanks, @kaydelaney for the cool refactor!
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-55314-to-v9.1.x origin/v9.1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 9e0d349bf95dfcb962da1b91294898495eca492a
# Push it to GitHub
git push --set-upstream origin backport-55314-to-v9.1.x
git switch main
# Remove the local backport branch
git branch -D backport-55314-to-v9.1.xThen, create a pull request where the |
(cherry picked from commit 9e0d349)
What this PR does / why we need it:
Adds a new prop to
FilterInputto disable regex escaping/unescaping for cases where it isn't needed, fixing an issue where search queries including special characters wouldn't return library panels that include those same characters.Which issue(s) this PR fixes:
Closes #55312
Special notes for your reviewer:
Hopefully some of the refactoring I did on
LibraryPanelsSearchis uncontroversial. I changed how state was handled from usinguseReducerto managing the state inline as I didn't really feel like there was an advantage compared to just usinguseState, but I'm more than happy to undo that change if someone can make a good argument for it.I did a bit more work than is seen here in this PR refactoring some of the related components, but I'll probably make a separate PR with those changes to avoid overloading this one.