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

step-style: add <a><paper-button> styling #51

Merged
merged 2 commits into from Apr 25, 2018
Merged

Conversation

x1ddos
Copy link
Contributor

@x1ddos x1ddos commented Apr 24, 2018

All this time we've had the following "download" paper-button structure
on the official website at https://g.co/codelabs:

<a href="..."><paper-button>download</paper-button></a>

Note that this is in accordance with the Polymer 1.0 recommendation
in https://elements.polymer-project.org/bower_components/paper-button/demo/index.html

Unfortunately, this is not how codelab-components were styled all this
time. A demo codelab has the inverse structure:

<paper-button><a href="...">download</a></paper-button>

Here's where it's used in codelab-components repo:

<p><paper-button raised class="colored"><iron-icon icon="file-download"></iron-icon><a href="https://chrome.google.com/webstore/detail/chrome-dev-editor-develop/pnoffddplpippgcfjdhbmhkofpnaalpg?hl=en">Download Chrome Dev Editor</a></paper-button></p></aside>

As a consequence, the styling Will provided in a recent change in f3e63d5 used the latter structure. We never noticed the difference because before the styling refresh, the download button background and link
colors were very close.

All this change really does is it removes the text underline. The rest of the styling already matches.

All this time we've had the following "download" paper-button structure
on the official website at g.co/codelabs:

    <a href="..."><paper-button>download</paper-button></a>

Note that this is in accordance with the Polymer 1.0 recommendation
in https://elements.polymer-project.org/bower_components/paper-button/demo/index.html

Unfortunately, this is not how codelab-components were styled all this
time. A demo codelab has the inverse structure:

    <paper-button><a href="...">download</a></paper-button>

Here's where it's used in codelab-components repo:
https://github.com/googlecodelabs/codelab-components/blob/7397f708d210dfbb7348084033a32ba070ec4df4/demo/codelab.html#L74

As a consequence, the styling Will provided in a recent change in
f3e63d5
used the latter structure. We never noticed the difference because
before the styling refresh, the download button background and link
colors were very close.
@x1ddos x1ddos requested a review from ebidel April 24, 2018 17:25
@@ -194,6 +194,13 @@
color: inherit !important;
}

.instructions ::content a paper-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

couldnt you just target .instructions ::content a and use text-decoration: none;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that would remove the underline from all regular links as well, wouldn't it?
I was convinced the underline text-decoration is encouraged in material design spec, but I'm far from an expert. Either way, we've always had underline on regular links, so I just wanted to preserve that styling.

Copy link
Contributor

@ebidel ebidel Apr 24, 2018

Choose a reason for hiding this comment

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

Right. I gotcha.

Do you need a text-align:center;?

screen shot 2018-04-24 at 10 46 52 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... maybe I'm looking in a difference place but I don't see that:

image

Also, I think text-align wouldn't play much role here because the button is supposed to grow up to the contents size, so I assumed it would also be centered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, I think I know what you mean. 1 sec...

Copy link
Contributor Author

@x1ddos x1ddos Apr 25, 2018

Choose a reason for hiding this comment

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

So, yeah the default computed text-align is start. Changing it to center or something else doesn't seem to have any effect though. I believe this is because there's actually no blank space to center it around and the way to center would be changing paddings/margins but I'm not sure this is a good idea. Besides, it doesn't look too bad:

image

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that looks fine. I was seeing it on the buttons without icons.

@ebidel
Copy link
Contributor

ebidel commented Apr 25, 2018

LGTM

@x1ddos x1ddos merged commit b885082 into master Apr 25, 2018
@x1ddos x1ddos deleted the paper-button-link branch April 25, 2018 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants