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

[Label] Inverted Variant, basic grouping, basic tag, bugfixes #230

Merged
merged 11 commits into from
Nov 27, 2018

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Oct 31, 2018

Description

  • added inverted variant (normal|basic|tag|corner|ribbon)
  • added support for grouping basic labels
  • added support for basic tag label
  • bug fixes
  • moved color generation into less macro for shorter code and easier maintainability
  • all color related !important statements (> 450 in previous compiled css 😱 ) are not necessary anymore and were removed 🙂

Screenshots

inverted_label_v2
inverted_basic_label_v2
basic_tag_label

Closes

#116
Semantic-Org/Semantic-UI#6639
Semantic-Org/Semantic-UI#3413

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/css Anything involving CSS state/awaiting-reviews Pull requests which are waiting for reviews state/awaiting-docs Pull requests which need doc changes/additions labels Oct 31, 2018
@y0hami
Copy link
Member

y0hami commented Nov 1, 2018

I'm not 100% on this, I don't think they should have borders.

@fomantic/admins what's your opinions?

@lubber-de
Copy link
Member Author

I'm not 100% on this, I don't think they should have borders.

Which element should not have borders? I adopted the Button-logic as requested in #116 and every inverted button has borders. 🤔

@y0hami
Copy link
Member

y0hami commented Nov 1, 2018

I should have been more clear when I said

like inverted buttons

I ment the colors used not the style and hover behavior.

@lubber-de
Copy link
Member Author

lubber-de commented Nov 1, 2018

OK, this could of course reduce complexity, but before I am touching this again, let us clearly decide how every element should behave. It's a bit of inconsistency again compared to other components.

Suggestion :

  • remove the hover from inverted label, instead always display them with full light background (what the hover currently does)
  • let basic inverted behave like the current inverted above instead (or remove the hovering there aswell, so it's always bordered)
  • keep basic inverted pointing as it is (bordered without any hovering)
  • add inverted pointing which displays them with full background
  • same for tags :only inverted tag has full light background and basic inverted tag is bordered but the hovering removed

@y0hami
Copy link
Member

y0hami commented Nov 1, 2018

I agree with apart from the fact that inverted labels will look a bit weird if they don't have any hover behavior. Let's wait to see what the others think.

prudho
prudho previously approved these changes Nov 2, 2018
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

Nothing really bother me there, looks pretty consistent.

@lubber-de lubber-de self-assigned this Nov 8, 2018
@y0hami
Copy link
Member

y0hami commented Nov 12, 2018

@lubber-de how about if you have the ui inverted label a solid color and on hover it changes to the normal color instead of the inverted version.

@lubber-de
Copy link
Member Author

@lubber-de how about if you have the ui inverted label a solid color and on hover it changes to the normal color instead of the inverted version.

@hammy2899 So the same behaviour as usual ui label, but the colors for normal/hover are swapped? sure. Everything else then as suggested above? If nobody else complaints (@prudho was still fine with the current solution anyway 😆), then i am going to adjust the PR accordingly (can't promise to get this done until wednesday for 2.6.4...but i'll try hard 😉)

@y0hami
Copy link
Member

y0hami commented Nov 12, 2018

@lubber-de Before you go ahead and start changing the PR have you tried using @lightRedHover etc.

@lubber-de
Copy link
Member Author

lubber-de commented Nov 12, 2018

I will use @lightRed / @lightRedHover instead of @lightRed / @red for hovering then .. Good hint.
Want me to wait until @prudho and @ColinFrick agree to the changes?

@y0hami
Copy link
Member

y0hami commented Nov 12, 2018

@lubber-de You can do if you want.

@ColinFrick
Copy link
Member

I'm on the fence here. We have to different approaches for inverted style: button (sui) and toast (fui)
As far as I understand the inverted style shouldn't change the style drastically, but it still does in the button module.

The question is, should we change the inverted button implementation accordingly to be more consistent with this implementation?

Additionally we also have inconsistency between the inverted input and inverted dropdown fields.
image
image

@lubber-de
Copy link
Member Author

lubber-de commented Nov 13, 2018

The question is, should we change the inverted button implementation accordingly to be more consistent with this implementation?

I vote for this. The current basic inverted button should also become the current inverted button.
It would be fully consistent with the above mentioned label changes then.

If we do this, then this inverted label PR should not be part of 2.6.4 but rather a 2.7.x, don't you think?

Additionally we also have inconsistency between the inverted input and inverted dropdown fields.

Yes, we still have the discussion at #158 😉

@y0hami
Copy link
Member

y0hami commented Nov 13, 2018

IMHO I would argue that buttons don't have an inconsistency but more that basic doesn't exactly make sense. Maybe we want to look at renaming basic to something like outlined button or bordered button. I do agree with the forms and we will need to do something about this in the future.

My main problem right now with the labels is that the current solution in this PR makes them look too much like a button. A label should show information not mislead a user into clicking it.

@ColinFrick
Copy link
Member

@hammy2899 We already have the problem with label / button:
image
image

Which are the buttons? 🤔

@lubber-de
Copy link
Member Author

lubber-de commented Nov 13, 2018

So, how should i proceed with this PR? Changing as suggested above and we'll see how to deal with the buttons afterwards? (This would be my suggestion)

@lubber-de
Copy link
Member Author

Btw: For the inverted toast i adopted the progress bar logic where the background color is still solid using the light background.
A transparent toast background would make the inside Text nearly unreadable (remember, we had the discussion about the opacity-setting for the same reason) and a always black background with just borders color can be easily accomplished by using ui inverted message there

That said i again suggest to adjust this PR according to the above mentioned changes, and let`s discuss/prepare the button afterwards. As @ColinFrick already pointed out: Button is the only component with a different inverted meaning

@y0hami
Copy link
Member

y0hami commented Nov 13, 2018

@lubber-de I agree, let's change this to use the inverted colors and have a discussion about buttons in discord maybe.

@lubber-de
Copy link
Member Author

2nd attempt pushed. 🙂 Updated screenshots in first comment accordingly.
I now used the light hover variables. Every label has a hovering now, but is only barely recognizable (especially for basic (tag|pointing) labels) because of the tight small change to the original SUI lightcolor variables.
Check it out and tell me if we have a winner now or again some changes are needed 😉

@y0hami
Copy link
Member

y0hami commented Nov 15, 2018

I think they look better now but I do agree they have very little hover effect. We may want to see what uses those light hover colors and see if making them more drastic would affect anything.

@y0hami y0hami added this to the 2.7.0 milestone Nov 15, 2018
@lubber-de
Copy link
Member Author

The @light{color}Hovervariables are currently only used in inverted header, so i think we can increase the darkening on those variables without any much hassle for a slightly better hover recognition

@y0hami
Copy link
Member

y0hami commented Nov 16, 2018

@lubber-de I agree.

@fomantic/admins If you disagree feel free to say.

@lubber-de lubber-de changed the base branch from beta to develop November 16, 2018 14:12
@lubber-de
Copy link
Member Author

lubber-de commented Nov 21, 2018

Here is a suggestion to increase the darkness of light{color}Hover which affects header aswell.
After some playing with the percentage of darken and saturate this only slight increase of darkness from 5 to 10 without changing saturation was the best recognizable solution without exxageration

Old

saturate(darken(@lightColor, 5), 10, relative);

old_lighthovercolor

New

saturate(darken(@lightColor, 10), 10, relative);

new_lighthovercolor

Tell me, if i should change it accordingly, differently or keep it as it is

@y0hami
Copy link
Member

y0hami commented Nov 21, 2018

@lubber-de I think that looks good, only thing I can think of changing is to make the text color white on the solid labels.

@lubber-de
Copy link
Member Author

@hammy2899

I think that looks good, only thing I can think of changing is to make the text color white on the solid labels.

🤔 Well, that would break the logic we now implemented in inverted toast and inverted progress, where we decided to keep every text color black (because white was more unreadable on light background colors). Let us not get inconsistent again and keep track with those decisions for toast and progress, thus keep the text-color black here aswell. Only exception in comparison to toast and progress: There is no hovering, so we could discuss about having white as text color only on hovering the inverted labels, but this will also break with the non-inverted label-hover logic: hovering over normal colored labels does not change the text-color. (@{color}TextColor is the same as @{color}HoverTextColor)

@y0hami
Copy link
Member

y0hami commented Nov 22, 2018

@lubber-de That's a good point, lets just leave it 😄

ColinFrick
ColinFrick previously approved these changes Nov 27, 2018
Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ColinFrick ColinFrick left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Nov 27, 2018
@y0hami y0hami merged commit 3df1d57 into fomantic:develop Nov 27, 2018
@lubber-de lubber-de deleted the feat/116/inverted_label branch November 27, 2018 10:30
@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Nov 30, 2018
@lubber-de
Copy link
Member Author

FUI issues will be closed once we release a new version where the issue is fixed. The SUI issues stay open unless they get fixed there aswell, but this is up to jack who maintains SUI on his own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants