Skip to content
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

UI: Reflect search in URL #16527

Open
wants to merge 11 commits into
base: next
Choose a base branch
from

Conversation

jh3y
Copy link
Contributor

@jh3y jh3y commented Oct 29, 2021

Issue: #16287

Integrate way to reflect search("filter") in URL bar in order to share the URL for hosted Storybook instances.

What I did

  • Grab the Sidebar Search initialQuery from URLSearchParams and use the filter value.
  • Tap into Downshift render to update the URL bar with window.history.replaceState.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Tests are under Search.test.js. Tests populating the input field in addition to update window.location via mocked window.history 👍

@nx-cloud
Copy link

nx-cloud bot commented Oct 29, 2021

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a62b0ff. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@jh3y jh3y force-pushed the reflect-search-in-url-16287 branch from f040895 to 3b19b49 Compare October 29, 2021 18:50
@jh3y
Copy link
Contributor Author

jh3y commented Oct 29, 2021

Can we add a hacktoberfest-accepted label to this PR? 🙏

@domyen
Copy link
Member

domyen commented Oct 29, 2021

Thanks for the PR @jh3y! I tagged in @ghengeveld who built search and knows about the URL structure.

@jh3y
Copy link
Contributor Author

jh3y commented Oct 29, 2021

Rad! Thanks @domyen 🙏
I don't think I've broken anything but we'll see 🤞 😅

@jh3y jh3y force-pushed the reflect-search-in-url-16287 branch from 29f5903 to d2b4929 Compare October 29, 2021 21:09
@jh3y jh3y force-pushed the reflect-search-in-url-16287 branch from d2b4929 to 026d267 Compare October 30, 2021 10:57
@shilman shilman changed the title feat: reflect search in URL [#16287] UI: Reflect search in URL Nov 1, 2021
@yannbf
Copy link
Member

yannbf commented Dec 6, 2021

Hey @ghengeveld could you please take a look at this whenever you've got time?

@stale
Copy link

stale bot commented Jan 9, 2022

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 9, 2022
@jh3y
Copy link
Contributor Author

jh3y commented Jan 10, 2022

Bumping this so stale doesn't close it for inactivity ✨

Happy New Year! 🎉

@stale stale bot removed the inactive label Jan 10, 2022
@yannbf
Copy link
Member

yannbf commented Jan 17, 2022

Bumping this so stale doesn't close it for inactivity ✨

Happy New Year! 🎉

Hey @jh3y thanks a lot for your patience and happy new year! I'll make sure this PR is reviewed this week

@ghengeveld
Copy link
Member

ghengeveld commented Jan 17, 2022

URL params should not be handled by UI components. Instead the filter (or perhaps query or search) param should be handled similarly to other UI query params like shortcuts. See initialUrlSupport in url.ts. A initialQuery property on the UI state object makes sense here.

@jh3y
Copy link
Contributor Author

jh3y commented Jan 17, 2022

I'll take another look at this. Thanks for the links @ghengeveld 🙏 Will take me a minute to refresh my context knowledge for this again 😄

@ghengeveld
Copy link
Member

Great, thanks. It's appreciated!

@jh3y jh3y force-pushed the reflect-search-in-url-16287 branch 4 times, most recently from 171f659 to 964f341 Compare January 18, 2022 12:46
@jh3y
Copy link
Contributor Author

jh3y commented Jan 18, 2022

Updated @ghengeveld 🚀

  • Passing through a filter key in the ui key of state.
  • Updated the tests with a mocked return for useStorybookState.

Only thing is that the core tests are timing out. Not sure if it's a case of giving them a rerun as it's related to something with Vue 🤔

Thanks! ʕ´•ᴥ•`ʔ

@github-actions github-actions bot deleted the branch storybookjs:next November 28, 2023 23:23
@github-actions github-actions bot closed this Nov 28, 2023
@JReinhold JReinhold reopened this Nov 29, 2023
@JReinhold JReinhold changed the base branch from release-8-0 to next November 29, 2023 07:38
@valentinpalkovic valentinpalkovic self-assigned this Dec 4, 2023
Copy link

nx-cloud bot commented Mar 26, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 58272ef. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@43081j
Copy link
Contributor

43081j commented May 8, 2024

i caught this up from next FYI, then realised a few things were broken (conflicts, a bug, etc).

not realising i was still mid-merge, i fixed those and reworked the tests (so it shows up as a merge commit but we'll squash it anyway i imagine when it lands).

TLDR of what I changed:

  • the tests now test <Search> directly (unrelated to the sidebar etc)
  • dropped the testing-library dependency so we resolve to the same one the rest of the monorepo uses (this was causing a test failure otherwise, for whatever reason, old version i guess)
  • we no longer update the URL or call setQueryParams on render, only now calling it on user input
  • moved the main logic out of onInput and into downshift's own change handler (resolves a bug where the x button would clear the input but not change the url)
  • use vitest instead of jest

@jh3y
Copy link
Contributor Author

jh3y commented May 8, 2024

i caught this up from next FYI, then realised a few things were broken (conflicts, a bug, etc).

not realising i was still mid-merge, i fixed those and reworked the tests (so it shows up as a merge commit but we'll squash it anyway i imagine when it lands).

TLDR of what I changed:

  • the tests now test <Search> directly (unrelated to the sidebar etc)
  • dropped the testing-library dependency so we resolve to the same one the rest of the monorepo uses (this was causing a test failure otherwise, for whatever reason, old version i guess)
  • we no longer update the URL or call setQueryParams on render, only now calling it on user input
  • moved the main logic out of onInput and into downshift's own change handler (resolves a bug where the x button would clear the input but not change the url)
  • use vitest instead of jest

Legend 🙏 A lot has changed since first opening this PR. Looking back on this, I got it rebased at whatever stage and then couldn't seem to get the various tests passing at that time.

Thanks for taking a look!

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive work bringing this up to date!!

code/ui/manager/src/components/sidebar/Search.tsx Outdated Show resolved Hide resolved
@yannbf
Copy link
Member

yannbf commented May 15, 2024

Amazing work!! The story failures in Chromatic are not related to this PR. I just made another PR that fixes them so once that's merged, let's just get the commits in here to fix that.
#27145

@43081j
Copy link
Contributor

43081j commented May 15, 2024

the current build failure seems to be mostly due to having stories with args.

im slightly baffled by this because the Sidebar stories do the same and work fine 🤔

both Sidebar.stories.tsx and Search.stories.tsx have exactly this line:

export const Simple: Story = {};

but the search stories fail type checking..

edit: think i got it! 🙏

edit2: yup, it builds 🎉

@yannbf
Copy link
Member

yannbf commented May 16, 2024

@43081j that other PR was merged so I updated this branch (and fixed a small merge conflict), it should all be good now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants