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

Added Algolia Docsearch to docs website #1365

Merged
merged 28 commits into from
Aug 4, 2023
Merged

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Jun 16, 2023

Changes

Added Algolia Docsearch to the /docs pages of the documentation website.

The hosted website was indexed automatically by the crawler. I did not change the default parameters set in Algolia. But with time, I hope to fine-tune those parameters for better search results.

Old screenshot [Video31.webm](https://github.com/iTwin/iTwinUI/assets/45748283/2c5ae2a1-3aa0-4e13-a611-7b18424b5eb8)

New screenshot:

Video33.webm

Testing

I tested it with a few simple queries. Later on, I hope to try more complex search queries and also try to see what search queries users are searching for to improve the results for those types of queries.

Docs

Added an Algolia search button to Header.astro

@r100-stack r100-stack requested a review from a team as a code owner June 16, 2023 19:01
@r100-stack r100-stack self-assigned this Jun 16, 2023
@r100-stack r100-stack requested a review from a team as a code owner June 16, 2023 19:01
@r100-stack r100-stack requested review from mayank99 and adamhock and removed request for a team June 16, 2023 19:01
@mayank99
Copy link
Contributor

in general, it looks good apart. few minor comments:

  • i would like to make sure the mobile layout looks good.
  • instead of linking the algolia js/css from a cdn, we can self-host it
  • can we use a custom search button that opens the algolia dialog? the algolia search button doesn't fit the look&feel.
  • can we make the colors inside the dialog more accessible?

@r100-stack r100-stack changed the base branch from dev to main June 25, 2023 18:29
@r100-stack
Copy link
Member Author

  • i would like to make sure the mobile layout looks good.

Here is how it looks on smaller screen widths:

Video32.webm
  • instead of linking the algolia js/css from a cdn, we can self-host it

I changed it to use yarn packages instead of the CDN url

  • can we use a custom search button that opens the algolia dialog? the algolia search button doesn't fit the look&feel.

I was thinking the default button might seem more intuitive for some users since they might used the default button on other doc websites that use Algolia DocSearch too. But I can change it if you feel so.

  • can we make the colors inside the dialog more accessible?

I changed the default Algolia colors to use the colors in _global.astro. (https://github.com/iTwin/iTwinUI/pull/1365/files#diff-f58c340d3e32a221844059fe1b76fc412a6668e448c2b16b3b93e93e228907f1R219-R225). Also, is there any alternative to using !important? If so, I can replace the !important with that.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

i'm ok with leaving the search button styling unchanged in the initial PR but i would like to change it at some point.

also, could you target dev branch instead of main?

@@ -10,6 +10,8 @@ import '@fontsource/noto-sans/600.css';
import '@fontsource/noto-sans/300.css';
import '@fontsource/noto-sans/800.css';

import '@docsearch/css';
Copy link
Contributor

@mayank99 mayank99 Jul 12, 2023

Choose a reason for hiding this comment

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

if you import this inside a style tag, you will be able to use cascade layers like this:

@import '@docsearch/css' layer(algolia);

this will avoid the need for !important when overriding variables

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the idea! I tried this but unfortunately could not get it working. Even with importing into the cascade layer, I found some styles applied outside the layer. I could not figure out why.

Here is the current code. Can you please take a look?
https://github.com/iTwin/iTwinUI/pull/1365/files#diff-595fc35b63ffd581acefb86333565cf14eb0a0747438807742a45f57f98e4ebdR18-R30

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug in astro that they are not interested in fixing. withastro/astro#5857

You might be able to work around it by adding the variables to body instead of :root.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I am now using body and I don't need !important anymore. The Algolia styles still appear outside the cascade layer, but the <style> that contains body (instead of :root) appears higher in the layer specificity list. Just wondering, why could that be the case?

Here is the screenshot where the first layer is the Algolia class, the second layer with body are my classes, the third layer with :root is the duplicate layer, and the fourth layer are the algolia styles inside the cascade layer.

image
image

Copy link
Contributor

Choose a reason for hiding this comment

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

are you asking why body variables take preference over :root? that's because they are lower in the dom tree, so they override anything above it, regardless of specificity.

the layer issue is also just a dev-only thing. i'd suggest inspecting prod build to confirm everything is working

yarn build --filter=website
cd apps/website && yarn preview

Copy link
Member Author

Choose a reason for hiding this comment

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

are you asking why body variables take preference over :root? that's because they are lower in the dom tree, so they override anything above it, regardless of specificity.

body has a specificity of (0, 0, 1) and :root has a specificity of (0, 1, 0). So I first was a bit confused regarding why body took preference over :root.

Upon looking in the inpect panel, I saw that the layer with body is higher list of layers in the inspect panel. But I am not sure why did the body layer appear higher than the :root layer and thus override the styles in the layer with :root.

the layer issue is also just a dev-only thing. i'd suggest inspecting prod build to confirm everything is working

Sounds good, will inspect prod version.

apps/website/.gitignore Outdated Show resolved Hide resolved
apps/website/src/components/Header.astro Outdated Show resolved Hide resolved
apps/website/src/pages/_global.astro Outdated Show resolved Hide resolved
@r100-stack r100-stack changed the base branch from main to dev July 12, 2023 18:36
@r100-stack
Copy link
Member Author

i'm ok with leaving the search button styling unchanged in the initial PR but i would like to change it at some point.

Great, then I will focus on some other PRs and come back to the button styling a little later.

also, could you target dev branch instead of main?

Sounds good, changed the target branch to dev

@mayank99 mayank99 mentioned this pull request Jul 14, 2023
21 tasks
@mayank99
Copy link
Contributor

One more thing i noticed the search results always go to the prod url. Can we make the urls use the current context? e.g. localhost vs dev url vs prod url

@mayank99
Copy link
Contributor

Cross posting Greta's comment from #1414 (review):

I have my OS in light mode so Algolia search button is light (probably does not belong in this PR but adding it anyways)
Screenshot 2023-07-18 at 12 05 19

Copy link
Contributor

@xman343 xman343 left a comment

Choose a reason for hiding this comment

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

Looks good.

Seconding Mayank, it would be better if the search results directed you to the localhost page instead of to product.

@r100-stack
Copy link
Member Author

r100-stack commented Aug 1, 2023

One more thing i noticed the search results always go to the prod url. Can we make the urls use the current context? e.g. localhost vs dev url vs prod url

I added the transformItems prop to remove url.origin (https://www.itwinui.bentley.com) from the url. (3236957)

@r100-stack
Copy link
Member Author

Cross posting Greta's comment from #1414 (review):

I have my OS in light mode so Algolia search button is light (probably does not belong in this PR but adding it anyways)
Screenshot 2023-07-18 at 12 05 19

I fixed the styling issues so that it displays correctly when prefers-color-scheme is light and also when it is dark. (7c9e00d)

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM, lets merge it after resolving conflicts.

will need to remember to add environment variables to pipeline

@r100-stack r100-stack added this pull request to the merge queue Aug 4, 2023
Merged via the queue into dev with commit 2b42aa4 Aug 4, 2023
12 of 14 checks passed
@r100-stack r100-stack deleted the rohan/algolia-docsearch branch August 4, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants