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

[website] Implement algolia redesign #28252

Merged
merged 47 commits into from
Sep 22, 2021
Merged

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Sep 10, 2021

close #26112
close #28415
close #28467

Summary

  • add @docsearch/react, css latest version
  • refactor some part AppSearch.js to use built-in keyboard handler from docsearch.
  • override styles to match branding

Preview: https://deploy-preview-28252--material-ui.netlify.app/ (Try the search bar)

@hbjORbj hbjORbj added the website Pages that are not documentation-related, marketing-focused. label Sep 10, 2021
@hbjORbj hbjORbj requested a review from siriwatknp September 10, 2021 07:09
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 10, 2021

No bundle size changes

Generated by 🚫 dangerJS against d1e7ecc

@hbjORbj hbjORbj changed the title [website] Implement algolia redesign [WIP] [website] Implement algolia redesign Sep 10, 2021
@siriwatknp
Copy link
Member

I think we should try the docsearch lib. https://autocomplete-experimental.netlify.app/docs/DocSearch

cc @oliviertassinari what do you think between custom style the current dropdown to modal and use @docsearch/react?

@mnajdova
Copy link
Member

I think we should try the docsearch lib. https://autocomplete-experimental.netlify.app/docs/DocSearch

Agree 👍

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 13, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 13, 2021
@danilo-leal danilo-leal self-requested a review September 13, 2021 19:20
@hbjORbj hbjORbj changed the title [WIP] [website] Implement algolia redesign [website] Implement algolia redesign Sep 14, 2021
@hbjORbj hbjORbj requested review from mnajdova and eps1lon September 14, 2021 08:13
@eps1lon eps1lon changed the base branch from next to master September 14, 2021 08:30
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 15, 2021
@siriwatknp siriwatknp mentioned this pull request Sep 16, 2021
4 tasks
@hbjORbj

This comment has been minimized.

@siriwatknp

This comment has been minimized.

docs/src/modules/components/Link.tsx Outdated Show resolved Hide resolved
const [focused, setFocused] = React.useState(false);
const theme = useTheme();
useLazyCSS(
'https://cdn.jsdelivr.net/npm/@docsearch/[email protected]/dist/style.min.css',
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why @docsearch/css is a dependency of @docsearch/react? Maybe we don't need this hook anymore.

Copy link
Member

@siriwatknp siriwatknp Sep 20, 2021

Choose a reason for hiding this comment

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

They just reexport from @docsearch/react/style in here but I don't see it is using in their component.

What if we import the module like this? so that if the package version updated we don't need to update multiple places.

import "@docsearch/react/style" // equal to @docsearch/css

Copy link
Member

Choose a reason for hiding this comment

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

What if we import the module like this? so that if the package version updated we don't need to update multiple places.

I am trying this

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

We need to keep this.

Copy link
Member

Choose a reason for hiding this comment

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

We can go with the current approach and open another PR for improvement?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 20, 2021
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

This is looking awesome. Nice work @hbjORbj and @siriwatknp!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 21, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It looks awesome! 🎉

@mnajdova mnajdova requested a review from eps1lon September 21, 2021 13:09
@mnajdova mnajdova removed the on hold There is a blocker, we need to wait label Sep 22, 2021
@mnajdova mnajdova merged commit 97b1709 into mui:master Sep 22, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The new search UX feels much better! I have left a couple of notes so we can follow-up to polish:

  • On mobile should we remove the border radius or add a little bit of margin to see the black overlay?

Screenshot 2021-09-22 at 10 39 17

  • On user agents with width scrollbar, the header moves:

scrollbar movement

Maybe an issue to open at https://github.com/algolia/docsearch or something for us to batch as we handle the Modal

Comment on lines +411 to +423
'&::-webkit-scrollbar-thumb': {
borderColor:
theme.palette.mode === 'dark'
? theme.palette.primaryDark[900]
: theme.palette.background.paper,
backgroundColor:
theme.palette.mode === 'dark'
? theme.palette.primaryDark[700]
: theme.palette.grey[500],
},
'&::-webkit-scrollbar-track': {
backgroundColor: theme.palette.background.paper,
},
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, we could remove this, and use colorScheme: theme.palette.mode at the root

Suggested change
'&::-webkit-scrollbar-thumb': {
borderColor:
theme.palette.mode === 'dark'
? theme.palette.primaryDark[900]
: theme.palette.background.paper,
backgroundColor:
theme.palette.mode === 'dark'
? theme.palette.primaryDark[700]
: theme.palette.grey[500],
},
'&::-webkit-scrollbar-track': {
backgroundColor: theme.palette.background.paper,
},

addStartScreen();
}
if (searchInput) {
const handleInput = (e) => {
Copy link
Member

Choose a reason for hiding this comment

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

Please no shortcuts

Suggested change
const handleInput = (e) => {
const handleInput = (event) => {

if (modal) {
// fade out transition
modal.style.opacity = 0;
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

clearTimeout?

items: [
{ name: 'Material Icons', href: '/components/material-icons/' },
{ name: 'Text Fields', href: '/components/text-fields/' },
{ name: 'Button', href: '/components/buttons' },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ name: 'Button', href: '/components/buttons' },
{ name: 'Button', href: '/components/buttons/' },

searchParameters={{
facetFilters: ['version:master', facetFilterLanguage],
}}
placeholder="Search..."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
placeholder="Search..."
placeholder="Search"

@siriwatknp
Copy link
Member

On mobile should we remove the border radius or add a little bit of margin to see the black overlay?

We don't have search on mobile 😂

@siriwatknp
Copy link
Member

will batch some minor fixes from @oliviertassinari in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Pages that are not documentation-related, marketing-focused.
Projects
None yet
7 participants