search

algolia / react-instantsearch

⚡️ Lightning-fast search for React and React Native applications, by Algolia.
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/react/
MIT License
1.97k stars 386 forks source link

fix(PoweredBy): add margin between text and Algolia logo #3665

Closed FabienMotte closed 1 year ago

FabienMotte commented 1 year ago

Summary

This PR adds a margin between text and logo to the <PoweredBy> widget component for React InstantSearch.

As you can see on this CodeSandbox, there is no margin:

image

We don't have this issue in the RIS Storybook because a margin is added by the Algolia theme while the CodeSanbox above uses the Satellite theme.

Result

A margin-left: 0.3rem is added on the SVG, overriding the class property added by the Algolia theme if present (like on Storybook).

image

Questions

Should we remove the class property in the Algolia theme since it will now be overridden by the style property?

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ad6504f5ceb1e384d8e05e9dadf8d6cba5afb17:

Sandbox Source
react-instantsearch-app Configuration
hooks-example Configuration
react-instantsearch-app (forked) PR
netlify[bot] commented 1 year ago

Deploy Preview for react-instantsearch ready!

Name Link
Latest commit 5ad6504f5ceb1e384d8e05e9dadf8d6cba5afb17
Latest deploy log https://app.netlify.com/sites/react-instantsearch/deploys/635a42c18ff445000912ad13
Deploy Preview https://deploy-preview-3665--react-instantsearch.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.

FabienMotte commented 1 year ago

I don't think we should hardcode styles in the component, this is a CSS concern and should belong to the themes.

@sarahdayan Another solution I see: should we delete the "Search by" text in RIS and use the PoweredBy logo (that includes the text in the SVG) so we don't have the spacing issue anymore and RIS is aligned with the InstantSearch specs?

Note: I need to update the specs for the PoweredBy widget since the logo has changed

Haroenv commented 1 year ago

That would be a breaking change for people who have changed the text though, as it would revert to the English / original text. I think we should do either this or nothing (nothing being a fine option)

FabienMotte commented 1 year ago

@Haroenv Indeed, I forgot that part is translatable. thanks!

Closing this PR in favor of instantsearch-specs#127