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.

Sign up for GitHub

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

Jump to bottom

fix(www): tweak docsearch to init algolia when tabbed into #23040

Merged
merged 2 commits into from
Apr 23, 2020
Merged

fix(www): tweak docsearch to init algolia when tabbed into #23040

merged 2 commits into from
Apr 23, 2020

Conversation

tbntdima
Copy link
Contributor

@tbntdima tbntdima commented Apr 12, 2020 •
edited
Loading

Basically this PR fixes a bug described in #21705.
You can find more details on the solution in the thread on an issue.

Fixes #21705

@tbntdima tbntdima requested a review from a team as a code owner April 12, 2020 08:18
@tbntdima tbntdima mentioned this pull request Apr 12, 2020
Closed
@pvdz
Copy link
Contributor

pvdz commented Apr 16, 2020

@madalynparker can you take a look at this, please? Calling .focus() makes me itchy but it kinda makes sense. Would love to have your insight on this.

@madalynrose
Copy link
Contributor

@madalynparker can you take a look at this, please? Calling .focus() makes me itchy but it kinda makes sense. Would love to have your insight on this.

haha that's a very old account from college I think 🙈 calling .focus() seems fine here since the bug is that it actually moves focus away from the element, which is disruptive and confusing.

@tbntdima
Copy link
Contributor Author

tbntdima commented Apr 16, 2020 •
edited
Loading

Note:
It's my first experience writing tests, would love some feedback. I know the code appeared to be pretty repetitive since we have 3 similar cases. If you know some better options, please share? Should I move common login into a separate function?

@pvdz
Copy link
Contributor

pvdz commented Apr 22, 2020

Fwiw, I like explicit tests. Abstractions tend to hide problems or cause superficial problems and false positives.

@pvdz
Copy link
Contributor

pvdz commented Apr 22, 2020

( Btw the new tests are not passing. Are they passing when you run yarn jest locally? You can run yarn jest www/src/components/search-form/__tests__/search-form.js to only test your file, and (for example) ``yarn jest www/src/components/search-form/tests/search-form.js -t "it initializes algola docsearch on form focus, and keeps focus"` to test one specific test case. )

@tbntdima
Copy link
Contributor Author

@pvdz thanks for noticing it. Tests used to pass locally. I cleared the cache and reinstalled node modules, and it failed. Should be fixed now 🙃

pvdz
pvdz approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks for this fix :D

@pvdz pvdz merged commit 73eb53b into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-docsearch branch April 23, 2020 08:54
pvhee added a commit to marzeelabs/gatsby that referenced this pull request Apr 24, 2020
@pvhee
Merge branch 'master' of github.com:gatsbyjs/gatsby
cc513b5 
* 'master' of github.com:gatsbyjs/gatsby: (64 commits)
  Fix recipe test problems (gatsbyjs#23347)
  create blog post announcing extended deadline for Virtual Gatsby Days… (gatsbyjs#23430)
  Correct CFP deadline date on Virtual Gatsby Days registration announcement (gatsbyjs#23432)
  fix(gatsby-recipes): Raise an error when an unknown resource is used (gatsbyjs#23428)
  feat(gatsby-recipes): While apply a step, show the time elapsed after 10 seconds (gatsbyjs#23362)
  markdownASTToHTMLAst isn't async (gatsbyjs#23427)
  Be more vocal about not doing type checking (gatsbyjs#23397)
  docs(gatsby-remark-images): mark `sizeByPixelDensity` as deprecated (gatsbyjs#23387)
  chore(all): Improve renovate (gatsbyjs#23411)
  chore(gatsby): count sift hits in telemetry (gatsbyjs#23416)
  chore(showcase): add GeneOS and COVID KPI (gatsbyjs#23405)
  feat(analytics): defer google analytics script (gatsbyjs#22806)
  docs: mention passing the .tsx file to createPage (gatsbyjs#23329)
  fix(www): tweak docsearch to init algolia when tabbed into (gatsbyjs#23040)
  chore(docs): Fix typo in url (gatsbyjs#23394)
  chore(gatsby-preset-gatsby-package): Remove tsconfig.json (gatsbyjs#23388)
  fix(gatsby-recipes): link to the raw gist of .estlintrc.js (gatsbyjs#23390)
  docs: Create gitlab-continuous-integration.md (gatsbyjs#23367)
  chore(doc): switch zeit now to Vercel Now for Gatsby deployment (gatsbyjs#23346)
  chore(showcase): add Resume on the Web (gatsbyjs#23371)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvdz pvdz pvdz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(www): Docsearch doesn't work when tabbed into.
4 participants
@tbntdima @pvdz @madalynrose @vladar