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] Right icon should have correct margin when placed after text #388

Merged
merged 7 commits into from
Jan 16, 2019

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Jan 13, 2019

Description

This PR is based on Semantic-Org/Semantic-UI#6210 by @w96k.

  • Add icon label variation that allows to put an icon without text.
  • Add right icon label variation that allows to put an icon in the right side of label with correct margin.
  • Add left icon label variation to let delete icon and close icon be placed in left with correct margin for backward-compatibility
  • Add hoverable class that makes icon to be used for action, as same style as current close/delete icon have.

Testcase

Before

https://jsfiddle.net/oLc3pqeu/

After

https://jsfiddle.net/y4L7cadb/

Screenshot (when possible)

Normal Label

image

Ribbon Label

image

Closes

#222
Semantic-Org/Semantic-UI#6210
Semantic-Org/Semantic-UI#5328
Semantic-Org/Semantic-UI#5446
Semantic-Org/Semantic-UI#5619
Semantic-Org/Semantic-UI#6210

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

  • Please rebase your branch to develop
  • The new invented code ui icon label and ui right icon label looks fine to me 🙂
  • Don't remove the delete and close settings (this will also have impact in multiple selection dropdown), just keep them in addition to your new invented hoverable class to avoid a breaking change
  • @deleteMargin has a different size than @iconDistance at least in the default theme: Keep this separated to avoid a breaking change

So i propose to change the PR to the following. Then it's fully backward compatible and not a breaking change anymore. Of course users have to add right icon to their labels where they want to have a right positioned icon, but they don't need to do this for any existing close or delete icons, so it's more an enhancement then

/* Removable and Hoverable label */
.ui.label > .close.icon,
.ui.label > .delete.icon,
.ui.label > .hoverable.icon {
  cursor: pointer;
  font-size: @deleteSize;
  opacity: @deleteOpacity;
  transition: @deleteTransition;
}

/* Backward compatible positioning */
.ui.label:not(.icon) > .close.icon,
.ui.label:not(.icon) > .delete.icon {
  margin: 0 0 0 @deleteMargin;
}

.ui.label > .delete.icon:hover,
.ui.label > .hoverable.icon:hover {
  opacity: 1;
}

/* Label for only an icon */
.ui.icon.label > .icon {
  margin: 0 auto;
}

/* Right Side Icon */
.ui.right.icon.label > .icon {
  margin: 0 0 0 @iconDistance;
}

See here:
https://jsfiddle.net/6Lmh2k9p/

@lubber-de lubber-de added lang/css Anything involving CSS 📝 needs test case tag/sui-issue Taken from an existing Issue/PR of SUI state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements and removed 📝 needs test case labels Jan 13, 2019
@lubber-de lubber-de added this to the 2.7.x milestone Jan 13, 2019
@exoego
Copy link
Contributor Author

exoego commented Jan 13, 2019

@lubber-de
Thanks for backward-compatibility suggestion!!

However, close/delete icon will have incorrect margin when placed before text like
image
because of margin defined in

/* Backward compatible positioning */
.ui.label:not(.icon) > .close.icon,
.ui.label:not(.icon) > .delete.icon {
  margin: 0 0 0 @deleteMargin;
}

Do you have idea to fix this and keep backward-compatibility all together?

BTW, there is times icon, as an alias of delete/close icon. So there is a workaround if users want to use x on left.
image

@lubber-de lubber-de self-requested a review January 13, 2019 14:05
@lubber-de
Copy link
Member

lubber-de commented Jan 13, 2019

@exoego Having a close/delete to the left of a label was never proposed part of the framework, so although this is a valid visual point, it's not a breaking change, because old used code (if anywhere used by somebody anyway) like

<div class="ui label">
    <i class="delete icon"></i>
  Delete me
</div>

will work/look (as ugly) as before

But it is a valid point. So to make this also work , what about inventing an explicit .ui.left.icon.label (for users who definately want to have those icons also displayed to the left)
adding it accordingly in label.less here:

/* Icon */
.ui.left.icon.label > .icon,
.ui.label > .icon {
  width: auto;
  margin: 0 @iconDistance 0 0;
}

Also in Addition:
Respect the different @deleteMargin, if those icons are used to the left

.ui.label.left.icon > .close.icon,
.ui.label.left.icon > .delete.icon {
  margin: 0 @deleteMargin 0 0;
}

See here: https://jsfiddle.net/6Lmh2k9p/1/

When doing FUI 3.0, we may take a look at it again to simplify the logic (at least getting rid of the separate @deleteMargin or have an additional class for it)

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

What do you think about my comment regarding the support of having close/delete to the left above?

src/definitions/elements/label.less Show resolved Hide resolved
@exoego
Copy link
Contributor Author

exoego commented Jan 13, 2019

I agree to keep backward compatibility as much as possible during v2.x.

Updated code, PR description, and example in docs(fomantic/Fomantic-UI-Docs#72).

lubber-de
lubber-de previously approved these changes Jan 14, 2019
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM now 🙂 Thanks for your patience

@exoego
Copy link
Contributor Author

exoego commented Jan 14, 2019

Removed hoverable variation to keep this PR scope small.

Initially I thought hoverable is required to keep delete/close icon style for backward-compatiblity and broaden the style to other icon.
However, it is no longer required for backward-compatibility.

I think that label module should focus on "A label displays content classification" as stated in the docs.
hoverable or such dynamic behavior is good to be implemented in button module.

@lubber-de lubber-de added the state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo label Jan 14, 2019
Copy link
Member

@lubber-de lubber-de 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

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

@y0hami y0hami merged commit 20d6389 into fomantic:develop Jan 16, 2019
@y0hami y0hami removed state/awaiting-reviews Pull requests which are waiting for reviews labels Jan 16, 2019
@lubber-de lubber-de modified the milestones: 2.7.x, 2.7.2 Jan 16, 2019
@exoego exoego deleted the right-icon-label branch January 16, 2019 13:09
y0hami pushed a commit to fomantic/Fomantic-UI-Docs that referenced this pull request Jan 17, 2019
@y0hami y0hami mentioned this pull request Feb 4, 2019
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 tag/sui-issue Taken from an existing Issue/PR of SUI type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants