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

[Tag] Add icon and rightIcon props #2431

Merged
merged 14 commits into from
Jun 11, 2018
Merged

[Tag] Add icon and rightIcon props #2431

merged 14 commits into from
Jun 11, 2018

Conversation

llorca
Copy link
Contributor

@llorca llorca commented Apr 27, 2018

image

@llorca llorca requested review from invliD and giladgray April 27, 2018 22:08
@blueprint-bot
Copy link

different rightIcon

Preview: documentation | landing | table

@dmackerman
Copy link
Contributor

dmackerman commented Apr 27, 2018

Super minor nit, the icon feels a bit...big? Might just be some visual trickery.

We have some tags with icons inside of them that end up looking like:
image

Just custom CSS to render the Icon at 12px.

@llorca
Copy link
Contributor Author

llorca commented Apr 27, 2018

you're right, but since 2.0, you can put any SVG in icon props, not just a Blueprint one. I highly discourage using a default 16px icon in there, or resizing a 16px one to 12. On our end, we use custom made 12px icons to ensure crispness

@llorca
Copy link
Contributor Author

llorca commented Apr 27, 2018

for some reason, when showing the left icon only (not the right one), the stray div container's height increases by 2px... any idea how to go around that?

@blueprint-bot
Copy link

remove unnecessary flex

Preview: documentation | landing | table

@giladgray
Copy link
Contributor

giladgray commented Apr 28, 2018

soooo this is already technically possible since you can just put JSX in <Tag> elements, so you can do <Tag><Icon />content<Icon /></Tag>. all we really need to do is fix the spacing, and i think the best way to do that is along the lines of how pt-flex-container() mixin will automatically put margin after all elements except the last, rather than using the align classes.

i'm happy to pick this up at some point next week.

@blueprint-bot
Copy link

Merge branch 'develop' of github.com:palantir/blueprint into al/tag-icons

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix tag-input line-height

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix tests

Preview: documentation | landing | table

@giladgray
Copy link
Contributor

🤔 multiline support is poor here

@giladgray giladgray added this to the 3.0.0 milestone Jun 4, 2018
@blueprint-bot
Copy link

add Tag multiline prop and Text tagName prop (to use span)

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix test

Preview: documentation | landing | table

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Just my opinion but the the tags seem to be too small to fit normal-sized icons.

Also, the contrast is too high and it doesn't look good until you enable minimal styling:
image

@@ -205,7 +205,6 @@ export const TAB_PANEL = `${TAB}-panel`;
export const TABS = `${TAB}s`;

export const TAG = `${NS}-tag`;
export const TAG_REMOVABLE = `${TAG}-removable`;
Copy link
Contributor

Choose a reason for hiding this comment

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

another breaking change to make note of in release notes

Copy link
Contributor

Choose a reason for hiding this comment

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

def, tho it only affects CSS consumers. #2587

@blueprint-bot
Copy link

remove tag-removable from docs

Preview: documentation | landing | table

@giladgray giladgray merged commit 9e3dc03 into develop Jun 11, 2018
@giladgray giladgray deleted the al/tag-icons branch June 11, 2018 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants