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

refactor(theme-search-algolia): migrate package to TS #5935

Merged
merged 13 commits into from
Nov 16, 2021
Merged

refactor(theme-search-algolia): migrate package to TS #5935

merged 13 commits into from
Nov 16, 2021

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Nov 12, 2021 •
edited
Loading

Motivation

#5817

Have you read the Contributing Guidelines on pull requests?

yes

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 12, 2021
@armano2 armano2 marked this pull request as draft November 12, 2021 17:17
@armano2
Copy link
Contributor Author

armano2 commented Nov 12, 2021 •
edited
Loading

There is small issue with migrating this package, useContextualSearchFilters is defined within theme-classic and theme-search-algolia doesn't rely and see this package

to fix this we have to do one of 2 thinks

  • theme-search-algolia has dependency on theme-classic
  • useContextualSearchFilters is moved to theme-common (this seem to be a better option)

i can fix it both ways, but I'm unsure witch one is preferred

cc @Josh-Cena

@armano2 armano2 marked this pull request as ready for review November 12, 2021 17:51
@armano2 armano2 marked this pull request as draft November 12, 2021 18:16
@netlify
Copy link

netlify bot commented Nov 12, 2021 •
edited
Loading

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 6a67df4

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/618f4ce0d83dbf00089a937d

😎 Browse the preview: https://deploy-preview-5935--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Nov 12, 2021 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 88
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5935--docusaurus-2.netlify.app/

@armano2 armano2 marked this pull request as ready for review November 12, 2021 23:45
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Nov 13, 2021
@Josh-Cena Josh-Cena added the status: blocked This issue is blocked by another issue or external dep and can't be pushed further. label Nov 14, 2021
@slorber
Copy link
Collaborator

slorber commented Nov 16, 2021

Thanks 👍 good enough to merge

Now that we have TS here it will be better to refactor a bit because it's quite messy 🤪

@slorber slorber merged commit 425144a into facebook:main Nov 16, 2021
@armano2 armano2 deleted the fix/refactor-theme-search-algolia branch November 16, 2021 19:37
@Josh-Cena Josh-Cena removed the status: blocked This issue is blocked by another issue or external dep and can't be pushed further. label Nov 17, 2021
@Josh-Cena Josh-Cena mentioned this pull request Nov 19, 2021
Closed
8 tasks
@jasonbosco jasonbosco mentioned this pull request Dec 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josh-Cena Josh-Cena Josh-Cena left review comments

@lex111 lex111 Awaiting requested review from lex111

@slorber slorber Awaiting requested review from slorber slorber is a code owner

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@armano2 @slorber @Josh-Cena @facebook-github-bot