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

[Item] Inverted Variant #232

Merged
merged 3 commits into from
Nov 5, 2018
Merged

Conversation

lubber-de
Copy link
Member

Description

Inverted variant of Item.

Screenshot

image

Closes

#109

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/css Anything involving CSS state/awaiting-docs Pull requests which need doc changes/additions labels Nov 2, 2018
@lubber-de lubber-de added the state/awaiting-reviews Pull requests which are waiting for reviews label Nov 2, 2018
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.

I don't think this should affect the background color, other inverted components only change text color and the background can be handled by the user via an inverted segment or custom CSS.

@lubber-de
Copy link
Member Author

So I am removing inverteditemsbackground (which is currently set to black) only then. Because for items, extra and content there already was a background setting in the non inverted variant aswell, but set to transparent or none(as already discussed in the related issue)

@lubber-de
Copy link
Member Author

@hammy2899 i removed the black background, so it's up to the user now to set it accordingly.
I currently kept the inverted background settings for item, content and extra (missed inverted anyway in the commits before) as they are set to the same value (transparent or none) as the normal item

If i should remove those aswell, then just tell me 🙂

@y0hami
Copy link
Member

y0hami commented Nov 2, 2018

Maybe we should look into why the background is set by the item before we move forward. If it turns out it isn't for any reason maybe we should just not set it.

@lubber-de
Copy link
Member Author

The latest commit supports texts directly in the .content element (normal or inverted) when used in .items or in .list aswell. Fixes Semantic-Org/Semantic-UI#6652

Regarding the background: Also in .list it seems every background setting was just prepared for someone to be able to change the background via less variables. That said also for .list the background- (and inverted-background) defaults are again always either transparent or none,

While reading through the code of all other elements/modules i could not see any other reason, why item needs to set the background (it was not even !important). So for backward compatibility we should not remove it.

So i think we should follow that for item alone too.

@y0hami
Copy link
Member

y0hami commented Nov 4, 2018

Ok with that being said then let's merge this.

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.

LGTM

@y0hami y0hami merged commit a52c665 into fomantic:beta Nov 5, 2018
@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Nov 5, 2018
@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 5, 2018
@lubber-de lubber-de added this to the 2.6.4 milestone Nov 5, 2018
@lubber-de lubber-de deleted the feat/109/inverted_item branch November 8, 2018 15:30
@y0hami y0hami mentioned this pull request Nov 14, 2018
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