Skip to content

fix(predictive-search): remove font from image #1595

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Maybach91
Copy link

@Maybach91 Maybach91 commented Apr 6, 2022

PR Summary:
There was an unnecessary font-family property on the search image with the content of 'object-fit: container', which is invalid at first and second we don’t really need font-family on the search image explicit.

Why are these changes introduced?

clean code

Testing steps/scenarios

  • No difference at production, just syntax sugar.

Checklist

There is a unnecessary font-family property on the search image with the content of `'object-fit: container'`, which is invalid at first and second we don’t really need font-family on the search image explicit.
@ghost ghost added the cla-needed label Apr 6, 2022
@ghost ghost removed the cla-needed label Apr 15, 2022
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Hello @Maybach91, thanks for looking Dawn and ways to make it better 🙂
In this case I think we had this declaration as a work around to support the css property object-fit in older browsers. Though it's only used in this instance 🤔
So I think we can actually get rid of it. Thanks for taking care of it.

Looks like most browser do support the property and don't need a work around: screenshot

@ludoboludo ludoboludo requested a review from martinamarien June 6, 2022 18:50
@martinamarien martinamarien removed their request for review March 6, 2024 15:21
@Maybach91
Copy link
Author

Any plan to merge this @ludoboludo @KaichenWang @martinamarien ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants