Skip to content
This repository has been archived by the owner. It is now read-only.

Fix Bug 1449338 - Show currently selected engine in newtabs search input #4097

Merged
merged 1 commit into from Apr 19, 2018
Merged

Fix Bug 1449338 - Show currently selected engine in newtabs search input #4097

merged 1 commit into from Apr 19, 2018

Conversation

@daleharvey
Copy link
Contributor

@daleharvey daleharvey commented Apr 17, 2018

This updates the CSS to handle the change in https://reviewboard.mozilla.org/r/237204/diff/#index_header

@Mardak
Copy link
Member

@Mardak Mardak commented Apr 18, 2018

Testing with the patch from bug 1449338 applied with default and dark theme:
screen shot 2018-04-18 at 10 37 31 am
screen shot 2018-04-18 at 10 40 27 am
screen shot 2018-04-18 at 10 41 02 am
screen shot 2018-04-18 at 10 40 01 am
screen shot 2018-04-18 at 10 43 54 am
screen shot 2018-04-18 at 10 39 25 am

And for comparison before:
screen shot 2018-04-18 at 10 50 36 am
screen shot 2018-04-18 at 10 50 21 am

@@ -2,11 +2,7 @@
$search-height: 35px;
$search-input-left-label-width: 35px;

This comment has been minimized.

@Mardak

Mardak Apr 18, 2018
Member

This should be renamed to something referring to icon. And is the sizing still correct given that the icon size is increasing from 16px to 18px? Perhaps we want something like

$search-icon-size: 18px;
$search-icon-padding: 8px;
$search-icon-width: 2 * $search-icon-padding + $search-icon-size;
@daleharvey
Copy link
Contributor Author

@daleharvey daleharvey commented Apr 19, 2018

Thanks for the review, updated and used the new vars in their appropriate places, looks cleaner now too, cheers

Copy link
Member

@Mardak Mardak left a comment

Was about to land this but then remembered rtl!

@@ -16,13 +15,15 @@
width: 100%;

input {
background: var(--newtab-textbox-background-color);
background: var(--newtab-textbox-background-color) var(--newtab-search-icon) $search-icon-padding center / $search-icon-size no-repeat;

This comment has been minimized.

@Mardak

Mardak Apr 19, 2018
Member

Oh oops! Almost forgot that the icon was centered before in the label, but now this is anchored to the left offset by $padding. For rtl, we'll want to offset from the right with the 2-value form: background-position-x: right $search-icon-padding;

image

@Mardak
Mardak approved these changes Apr 19, 2018
Copy link
Member

@Mardak Mardak left a comment

Thanks!

@Mardak Mardak merged commit 9ea1416 into mozilla:master Apr 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants