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

fix: adjust spacing for custom search properly #7164

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Apr 12, 2022

Motivation

Adjustment styles must be applied equally to all search blocks, not just from the Algolia one. Otherwise, the lack of proper styling in other custom search bars will lead to the following result:

image
https://nomicon.io/ as example

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview (no visual changes, it just adds search wrapper to all search bars)

Related PRs

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Apr 12, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 12, 2022
@netlify
Copy link

netlify bot commented Apr 12, 2022

[V2]

Name Link
🔨 Latest commit 485ace8
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6255de4ddbe5800008ac7e63
😎 Deploy Preview https://deploy-preview-7164--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 63
🟢 Accessibility 100
🟠 Best practices 83
🟢 SEO 100
🟢 PWA 90

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

@github-actions
Copy link

Size Change: +194 B (0%)

Total Size: 797 kB

Filename Size Change
website/build/assets/js/main.********.js 603 kB +194 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/assets/css/styles.********.css 106 kB
website/build/index.html 38.6 kB

compressed-size-action

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

I think this won't work as soon as we use the type: "search" navbar item, right? Nvm, looked at it again, search items are respected as well, but the point below still stands

IMO the goal is to reduce coupling between navbar items and navbar layout instead of hardcoding more special items, because it's in our plan to make every navbar item configurable (see also #7154 for one such confusion)

What about unifying the padding/margin for every navbar item? We have a similar problem for the color mode toggle as well.

@lex111
Copy link
Contributor Author

lex111 commented Apr 13, 2022

Since SeachBar implementation comes from third-party plugins, we need put it in wrapper to set proper spacing by analogy with other navbar items. I don't know any other way to achieve this without introducing extra component.

@Josh-Cena
Copy link
Collaborator

Oh, I see where you are coming from. Can this wrapper be made generic? e.g. a <NavbarItem> wrapper with a margin that wraps all kinds of links?

@lex111
Copy link
Contributor Author

lex111 commented Apr 13, 2022

No, because the search bar requires extra wrapper, not only to adjust the spacing, but to proper positioning on mobiles (see styles.css file).

@slorber
Copy link
Collaborator

slorber commented Apr 14, 2022

👍 looks like a useful extra wrapper

Agree we can still improve things more and having something like NavbarItemWrapper could be useful in the future, open to further improvements

@slorber slorber merged commit fe064a8 into main Apr 14, 2022
@slorber slorber deleted the lex111/spacing-search-custom branch April 14, 2022 09:53
@JPeer264
Copy link
Contributor

JPeer264 commented May 5, 2022

I totally understand the extra wrapper und support it, but is there a way of adding extra styles on these modules? Adding an extra className property is not supported when having type: 'search'. The reason why I ask this is, because we had some extra styles on .dsla-search-wrapper and is breaking it a little as we have another wrapper on top of it.

Screenshot 2022-05-05 at 08 30 30

@slorber
Copy link
Collaborator

slorber commented May 5, 2022

that seems reasonable to me that we allow a className attribute like other navbar items

Do you want to send a PR ?

@JPeer264
Copy link
Contributor

JPeer264 commented May 6, 2022

Sure I can add a PR there.

@slorber
Copy link
Collaborator

slorber commented Jun 1, 2022

Relevant issue in case there's too much spacing after upgrading: #7536

@JPeer264 didn't you encounter this problem with docusaurus-search-local ?

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.

5 participants