-
Notifications
You must be signed in to change notification settings - Fork 124
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
Update algolia plugin #1730
Update algolia plugin #1730
Conversation
Thanks for tackling this @jonahtanjz
I'm OK with it, if they haven't provided an easy way to use the dropdown for search results. |
74420d1
to
b33d585
Compare
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.
LGTM! 🎉
packages/core-web/src/index.js
Outdated
height: calc(100% - ${headerHeight}px - 66px); | ||
} | ||
}`, | ||
); |
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.
Just a quick look, would it be better to keep these strictly to the plugin itself? Since docsearch isn't really core functionality, but a plugin.
Some questions release wise:
- whether its possible to deliver a more minimal fix by adding the
apiKey
you mentioned ontov3.1.1
or so (also so no immediate action needed on author end to migrate v2 -> v3) - while we merge this bigger change into the main branch for
v4.0
(since v2 -> v3 docsearch is a breaking change)
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.
Just a quick look, would it be better to keep these strictly to the plugin itself? Since docsearch isn't really core functionality, but a plugin.
I've shifted the plugin css code to within the plugin file. The rest of the plugin code related to adjustHeaderClasses
is left there to minimise duplication as there are a couple of conditions that will need to adjust the plugin's css.
whether its possible to deliver a more minimal fix by adding the apiKey you mentioned onto v3.1.1 or so (also so no immediate action needed on author end to migrate v2 -> v3)
while we merge this bigger change into the main branch for v4.0 (since v2 -> v3 docsearch is a breaking change)
Yup agree with this. We should do the v2 -> v3 docsearch in our v4.0 and release the minimal fix in v3.1.1. I'll create a separate branch called v3.1.x-hotfixes
(similar to the v2.7.x-hotfixes branch) and push the update there. Is this workflow fine?
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.
Yup agree with this. We should do the v2 -> v3 docsearch in our v4.0 and release the minimal fix in v3.1.1. I'll create a separate branch called v3.1.x-hotfixes (similar to the v2.7.x-hotfixes branch) and push the update there. Is this workflow fine?
👍
Alternatively there haven't been any breaking changes since the last release so far, so we could also merge the more minimal fix directly on the main branch. (I think either is fine)
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.
I've shifted the plugin css code to within the plugin file. The rest of the plugin code related to adjustHeaderClasses is left there to minimise duplication as there are a couple of conditions that will need to adjust the plugin's css.
V3 has changed its design drastically from V2. The search bar has a new design and clicking on the search bar will open a modal instead. The searches will be done on this modal and the results will be displayed on the modal instead of the dropdown that is used by Markbind's search. Since the new search bar and modal are auto generated by Algolia's package, it will be complicated to try to make the design similar to the current Markbind's search. Is it fine to stick to their native design?
*For v4
Sorry, I completely missed the purpose of these changes earlier.
Are the dynamic styles added to tackle the above mentioned issue?
I'm favouring sticking with algolia's default design as much as possible if so, imo there isn't much benefit from pushing the dropdown / ui downward. Coupling our dynamic style detections over to an external element might break as well if algolia's styles / classes / dom structure changes.
If its an issue with correctness (e.g. the modal going out of the screen) I'll need a little more time to review this with your algolia setup
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.
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.
Perhaps we can do a minor release after this is merged? I would like to start using this in the module website ASAP. |
97bf3a5
to
63c3d9f
Compare
58f6f06
to
24c919b
Compare
@@ -42,18 +42,19 @@ function detectAndApplyFixedHeaderStyles() { | |||
|
|||
const headerHeight = headerSelector.height(); | |||
const bufferHeight = 1; | |||
insertCss(`.fixed-header-padding { padding-top: ${headerHeight}px !important }`); | |||
insertCss(`.fixed-header-padding { padding-top: ${headerHeight}px !important; }`); |
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.
Nice work @jonahtanjz, but do try to avoid the unnecessary changes here!
Particularly as there are no functional changes in the entire file
What is the purpose of this pull request?
Fixes #1729
Overview of changes:
Update Algolia from v2 to v3. Algolia now requires
appId
to be provided in order to use its APIs. V3 also requires that the search box be adiv
and no longer supportsinput
.User facing changes include adding new options and removing unsupported options:
Added:
appId
- compulsory fieldsearchParameters
- optionalRemoved:
debug
algoliaOptions
Anything you'd like to highlight / discuss:
V3 has changed its design drastically from V2. The search bar has a new design and clicking on the search bar will open a modal instead. The searches will be done on this modal and the results will be displayed on the modal instead of the dropdown that is used by Markbind's search. Since the new search bar and modal are auto generated by Algolia's package, it will be complicated to try to make the design similar to the current Markbind's search. Is it fine to stick to their native design?
New search bar:
![image](https://user-images.githubusercontent.com/47470981/150272880-6d22d417-0312-4189-a84a-4ad78182d88c.png)
New search modal instead of dropdown:
![image](https://user-images.githubusercontent.com/47470981/150273260-b6842168-7420-4e01-8162-38bafa67e808.png)
Testing instructions:
Add the following to
site.json
and serve the site.Proposed commit message: (wrap lines at 72 characters)
Update Algolia plugin
Algolia docsearch v2 is now a legacy version. Algolia also
requires new parameters for their APIs.
Let's update the version of Algolia docsearch to v3 and change
the plugin options accordingly.
Checklist: ☑️