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

Fixes #981 - adding a link for the labels in individual issue page #1081

Merged
merged 3 commits into from
Jun 30, 2016

Conversation

deepthivenkat
Copy link
Member

@miketaylr
Copy link
Member

Functionally, this is great!

I think we need to tweak the design though,

Default:
screen shot 2016-06-09 at 1 19 45 pm

Hover:
screen shot 2016-06-09 at 1 19 34 pm

We can override the opacity 0.7 that's getting inherited, I think it would be better as 1. And probably the underline can just be white instead of black.

Can you also add an Intern test to this PR, so we don't break this in a refactor at some point in the future?

@magsout
Copy link
Member

magsout commented Jun 11, 2016

@deepthivenkat good 👍

But I think, you can keep the default className and replace <span> by the <a> like :

<a 
  href="/issues?q=label:<%= label.remoteName %>"
  class="wc-Label wc-Label--badge js-Label" 
  data-remotename="<%= label.remoteName %>"
  title="Labels : <%= label.name %>"
  style="background-color:#<%=label.color%>"
>
  <%= label.name %>
</a>

We need to add text-decoration: none in wc-Label

@miketaylr
Copy link
Member

@deepthivenkat ping

@deepthivenkat
Copy link
Member Author

@miketaylr @magsout I have to make the underline color as white. This is normally done by adding text-decoration-color property which is deprecated.

Instead I have made use of border-bottom-color property for hover event.

@miketaylr
Copy link
Member

This is normally done by adding text-decoration-color property which is deprecated.

So it turns out this isn't deprecated, but is only supported in Firefox and Safari (with a prefix).

@deepthivenkat
Copy link
Member Author

Attaching a gif to show the css changes
label

r? @magsout

@miketaylr
Copy link
Member

I don't think we want the labels to change size when they're hovered over, it's sort of jarring.

Also the gray and yellow text doesn't look very good (it's hard for me to read).

@deepthivenkat
Copy link
Member Author

label2

@miketaylr how about now?

@miketaylr
Copy link
Member

Much better! Why are we changing the text color from white to gray?

@deepthivenkat
Copy link
Member Author

animation
Now?

@deepthivenkat
Copy link
Member Author

@miketaylr Grey was the default color. The CSS property used for font color:

color: inherit

@miketaylr
Copy link
Member

@miketaylr Grey was the default color. The CSS property used for font color:

Ah, gotcha. This looks better to me -- let's let @magsout review. :)

@deepthivenkat
Copy link
Member Author

Oki!

@miketaylr
Copy link
Member

Ah, I'm sorry -- I goofed here. We intentionally changed the color from white in #1094. >___<

@deepthivenkat
Copy link
Member Author

No problem! changed it back to grey

@deepthivenkat deepthivenkat force-pushed the issues/981/1 branch 3 times, most recently from 4b9a40b to 935ce6d Compare June 29, 2016 04:35
@magsout
Copy link
Member

magsout commented Jun 30, 2016

@miketaylr @deepthivenkat

I think previous style is better:

http://cl.ly/170o3B2Y4122

Hover with opacity is much better IHMO.

I have remove, border and decrease font-size:

.wc-Labels {
    display: inline-block;
    font-size: .8rem;
    font-weight: 700;
    border-color: transparent;
    opacity: .7;
    text-decoration: none;
    color: inherit;
    margin-left: .5em;
    padding: 0.2em;
    border-radius: 4px;
}

@miketaylr
Copy link
Member

@magsout agreed, seems nicer.

@miketaylr
Copy link
Member

Let's go ahead and merge this, thanks @deepthivenkat!

@miketaylr miketaylr merged commit b5fd80f into master Jun 30, 2016
@miketaylr miketaylr deleted the issues/981/1 branch June 30, 2016 16:56
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.

4 participants