Skip to content

fix(v2): update SearchPage styling, fix appearance in dark mode#3837

Merged
lex111 merged 5 commits intofacebook:masterfrom
Simek:update-search-page-ui
Nov 29, 2020
Merged

fix(v2): update SearchPage styling, fix appearance in dark mode#3837
lex111 merged 5 commits intofacebook:masterfrom
Simek:update-search-page-ui

Conversation

@Simek
Copy link
Contributor

@Simek Simek commented Nov 27, 2020

Motivation

Currently SearchPage styling feels a bit plain and in addition to that inputs and separator lines do not change appearance between the modes.

This PR updates the SearchPage component and introduces the following changes:

  • more CSS variable used from Infima and Docsearch itself
  • inputs borrows few styling properties the search input style from the modal
  • chevron sign (breadcrumbs splitter) received it's own class to make customizing the style (or even changing it via content) possible through CSS

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

The changes have been tested using Docusaurus V2 website locally.

Preview

search

Mobile

Screenshot 2020-11-28 005238

Related PRs

@Simek Simek requested review from lex111 and slorber as code owners November 27, 2020 23:34
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 27, 2020
@netlify
Copy link

netlify bot commented Nov 27, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 77b53b3

https://deploy-preview-3837--docusaurus-2.netlify.app

@netlify
Copy link

netlify bot commented Nov 27, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 350e9f8

https://deploy-preview-3837--docusaurus-2.netlify.app

@netlify
Copy link

netlify bot commented Nov 28, 2020

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 706409f

https://deploy-preview-3837--docusaurus-2.netlify.app

@Simek
Copy link
Contributor Author

Simek commented Nov 28, 2020

This is how full focus/active style looks when it was fully based on the Docsearch CSS variables, but I personally don't liked the background color of unselected element. In the other hand implementing border color change (on previous mentions interactions) might be a good idea, but this behavior will differ from the search input in modal, which is stylized as always focused.

Untitled

@lex111
Copy link
Contributor

lex111 commented Nov 28, 2020

inputs borrows few styling properties the search input style from the modal

I don't like this, it feels like inputs are always in focus/hover state. Typically, similar styling is used for outline property (which I actually plan to do in Infima, CSS outline based on the primary color). Moreover, resetting an outline is very bad practice.

@Simek
Copy link
Contributor Author

Simek commented Nov 28, 2020

I don't like this, it feels like inputs are always in focus/hover state. Typically, similar styling is used for outline property (which I actually plan to do in Infima, CSS outline based on the primary color). Moreover, resetting an outline is very bad practice.

@lex111 This was exactly my concern which I had stated in the comment above, you should explain why it's a bad approach and why you don't like this to Algolia devs, since they are the authors of DocSearch styling, not me. 😉

So I'm going to push the alternative interaction styling, which I have proposed above, since you have not recommended other alternatives.

@lex111
Copy link
Contributor

lex111 commented Nov 28, 2020

I got you, but Algolia search is a third-party project, although the search page uses the Algolia API, this is our feature, so we style it according to our styles (i.e. those defined in Infima), and not follow Algolia design guidelines.
I suggest simply leaving the default styling for inputs (from Infima), this is the most appropriate option, in my opinion.

@Simek
Copy link
Contributor Author

Simek commented Nov 28, 2020

although the search page uses the Algolia API, this is our feature, so we style it according to our styles

I can agree that it is Docusaurus code, but this plugin is 100% related and was created only to be used with Alogia, as even the package name suggest - theme-search-algolia.

Also this sentence is not true according to the search modal, which is not styled by Infima in any way, you only overwrite some DocSearch CSS variables to match the colors and the default structure and design of modal in not altered in any way.

I suggest simply leaving the default styling for inputs (from Infima), this is the most appropriate option, in my opinion.

But there is currently almost no styling (in/from Infima), and as stated before, inputs are looking incorrect in dark mode.

@Simek
Copy link
Contributor Author

Simek commented Nov 28, 2020

Updated preview

With borders (inset shadow) transition and no background altering.
search

@Simek
Copy link
Contributor Author

Simek commented Nov 28, 2020

I suggest simply leaving the default styling for inputs (from Infima)

@lex111 Can you elaborate more on the Infima default inputs styling, what do you have in mind? I'm was not able to find any mention about input styles in the Infima docs:

Only traces about that some styling exist in Infima are variables like:

  • --ifm-navbar-search-input-color
  • --ifm-navbar-search-input-background-color

But they are not general variables, they were created specifically for the Navbar search input.

@lex111
Copy link
Contributor

lex111 commented Nov 28, 2020

I understand what you want to do, but it's better to add dark mode support for input in Infima itself, rather in search page component. Especially now box-shadow is almost useless, because it is overlapped by the outline.
It seems that next week Infima will be opensourced so you can suggest appropriate changes.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Seems good.

@lex111 lex111 merged commit f43781f into facebook:master Nov 29, 2020
@lex111
Copy link
Contributor

lex111 commented Nov 29, 2020

@Simek thank you!

@Simek
Copy link
Contributor Author

Simek commented Nov 29, 2020

Especially now box-shadow is almost useless, because it is overlapped by the outline.

@lex111 That was the reason why I have disabled the outline in the initial version, but I have put it back as you suggested that is a bad practice. I also don't like the appearance of box-shadow and outline used at the same time. Maybe you will have a better idea how to improve this within incorporation of Infima styling.

Also I have got one more question, what is the reason behind using italic style for the result item summary? AFAIK there are no other italic styled text in theme-classic components.

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2020

Maybe you will have a better idea how to improve this within incorporation of Infima styling.

The solution is really simple - just use border instead of box-shadow, which can be removed on focus/hover state. This is best done in Infima, as it might come in handy for other inputs.

Also I have got one more question, what is the reason behind using italic style for the result item summary? AFAIK there are no other italic styled text in theme-classic components.

I don’t remember exactly why I did this, maybe I was inspired by some other search page. So maybe we should remove this, but I'm not sure about that.

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants