[CUPS] Modify the "Add Printer" link to a button.

[CUPS] Modify the "Add Printer" link to a button.

BUG=714697
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2867563002
Cr-Commit-Position: refs/heads/master@{#470103}
Committed: https://chromium.googlesource.com/chromium/src/+/a4b69e228383bae8fb9c44ce9b11e7800766ed55

[xdai1, 2017-05-05 22:10:55]

Description was changed from

==========
[CUPS] Modify the "Add Printer" link to a button.

BUG=714697
==========

to

==========
[CUPS] Modify the "Add Printer" link to a button.

BUG=714697
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[xdai1, 2017-05-05 22:20:18]

xdai@chromium.org changed reviewers:
+ michaelpg@chromium.org

[xdai1, 2017-05-05 22:20:19]

michaelpg@, could you help review this CL please? Thanks!

[michaelpg, 2017-05-05 22:53:38]

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printers.html:23:
paper-button {
What does this do differently that it needs to look different than other
Settings buttons? settings_shared_css.html has styling options -- would the
primary-button class work?

[xdai1, 2017-05-06 00:02:42]

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printers.html:23:
paper-button {
On 2017/05/05 22:53:38, michaelpg wrote:
> What does this do differently that it needs to look different than other
> Settings buttons? settings_shared_css.html has styling options -- would the
> primary-button class work?

I didn't notice that there was pre-defined "primary-button" class in the code
base. However, when I tried to apply the class to the button as you suggested, I
encountered a problem that I don't know how to solve and I hope you can give me
some suggestion. 

Since this button needs to be on the right side of the page, I used
"margin-left: auto" to implement this: use this in a flex container can push a
flex item to right (see https://www.w3.org/TR/css-flexbox-1/#auto-margins). But
if I apply the class "primary-button" to the button, it inherits a property
"-webkit-margin-start" from "primary-button" which prevents "margin-left: auto"
from working here. Overriding this property seems not working. The only way to
make "margin-left: auto" work is to remove "-webkit-margin-start" from the class
"primary-button".

[michaelpg, 2017-05-06 01:21:50]

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printers.html:23:
paper-button {
On 2017/05/06 00:02:42, xdai1 wrote:
> On 2017/05/05 22:53:38, michaelpg wrote:
> > What does this do differently that it needs to look different than other
> > Settings buttons? settings_shared_css.html has styling options -- would the
> > primary-button class work?
> 
> I didn't notice that there was pre-defined "primary-button" class in the code
> base. However, when I tried to apply the class to the button as you suggested,
I
> encountered a problem that I don't know how to solve and I hope you can give
me
> some suggestion. 
> 
> Since this button needs to be on the right side of the page, I used
> "margin-left: auto" to implement this: use this in a flex container can push a
> flex item to right (see https://www.w3.org/TR/css-flexbox-1/#auto-margins).
But
> if I apply the class "primary-button" to the button, it inherits a property
> "-webkit-margin-start" from "primary-button" which prevents "margin-left:
auto"
> from working here. Overriding this property seems not working. The only way to
> make "margin-left: auto" work is to remove "-webkit-margin-start" from the
class
> "primary-button". 

try -webkit-margin-start: auto instead of margin-left (i would recommend that
anyway, as the RTL-friendly version)

[xdai1, 2017-05-08 17:20:42]

michaelpg@, I've addressed your comment. Please take another look, thanks!

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/printing_page/cups_printers.html (right):

https://codereview.chromium.org/2867563002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/printing_page/cups_printers.html:23:
paper-button {
On 2017/05/06 01:21:50, michaelpg wrote:
> On 2017/05/06 00:02:42, xdai1 wrote:
> > On 2017/05/05 22:53:38, michaelpg wrote:
> > > What does this do differently that it needs to look different than other
> > > Settings buttons? settings_shared_css.html has styling options -- would
the
> > > primary-button class work?
> > 
> > I didn't notice that there was pre-defined "primary-button" class in the
code
> > base. However, when I tried to apply the class to the button as you
suggested,
> I
> > encountered a problem that I don't know how to solve and I hope you can give
> me
> > some suggestion. 
> > 
> > Since this button needs to be on the right side of the page, I used
> > "margin-left: auto" to implement this: use this in a flex container can push
a
> > flex item to right (see https://www.w3.org/TR/css-flexbox-1/#auto-margins).
> But
> > if I apply the class "primary-button" to the button, it inherits a property
> > "-webkit-margin-start" from "primary-button" which prevents "margin-left:
> auto"
> > from working here. Overriding this property seems not working. The only way
to
> > make "margin-left: auto" work is to remove "-webkit-margin-start" from the
> class
> > "primary-button". 
> 
> try -webkit-margin-start: auto instead of margin-left (i would recommend that
> anyway, as the RTL-friendly version)

It worked, thanks a lot!

[michaelpg, 2017-05-08 18:02:00]

lgtm

[xdai1, 2017-05-08 18:48:20]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2017-05-08 18:49:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 20:22:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494269300148530,
"parent_rev": "58676cfe8124067c5e2945f235f47ea9e1ec7f6e", "commit_rev":
"a4b69e228383bae8fb9c44ce9b11e7800766ed55"}

[commit-bot: I haz the power, 2017-05-08 20:22:56]

Description was changed from

==========
[CUPS] Modify the "Add Printer" link to a button.

BUG=714697
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CUPS] Modify the "Add Printer" link to a button.

BUG=714697
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2867563002
Cr-Commit-Position: refs/heads/master@{#470103}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4b69e228383bae8fb9c44ce9b11...
==========

[commit-bot: I haz the power, 2017-05-08 20:22:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a4b69e228383bae8fb9c44ce9b11...