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

Expose more CSS classes in JS #691

Merged
merged 2 commits into from
Feb 17, 2017
Merged

Expose more CSS classes in JS #691

merged 2 commits into from
Feb 17, 2017

Conversation

tgreenwatts
Copy link
Contributor

Fixes #0000

Checklist

Changes proposed in this pull request:

Add pt-icon and pt-text-overflow-ellipsis to Classes

Reviewers should focus on:

Not sure if pt-text-overflow-ellipsis should be considered a "modifier" or in a new category

@giladgray for review

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @tgreenwatts! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@blueprint-bot
Copy link

Add more to Classes

Preview: docs
Coverage: core | datetime

@adidahiya adidahiya changed the title Add more to Classes Expose more CSS classes in JS Feb 16, 2017
@@ -24,6 +24,7 @@ export const FIXED = "pt-fixed";
export const FIXED_TOP = "pt-fixed-top";
export const VERTICAL = "pt-vertical";
export const ROUND = "pt-round";
export const TEXT_OVERFLOW_ELLIPSIS = "pt-text-overflow-ellipsis";
Copy link
Contributor

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 is a modifier, more like a utility class. I'll let @giladgray confirm!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a new section:

...
export const ROUND = "pt-round";

// text utilities
export const TEXT_OVERFLOW_ELLIPSIS = "pt-text-overflow-ellipsis";

// components
...

Copy link
Contributor

@cmslewis cmslewis Feb 17, 2017

Choose a reason for hiding this comment

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

@palantir/blueprint Should we just put every CSS API class into this file? I'm thinking we might as well (EDIT: not in this PR, in a new one).

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to get autocomplete for the other ones in the typography. If we're doing one, no reason not to do the others too if we're maintaining this file

@@ -24,6 +24,7 @@ export const FIXED = "pt-fixed";
export const FIXED_TOP = "pt-fixed-top";
export const VERTICAL = "pt-vertical";
export const ROUND = "pt-round";
export const TEXT_OVERFLOW_ELLIPSIS = "pt-text-overflow-ellipsis";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a new section:

...
export const ROUND = "pt-round";

// text utilities
export const TEXT_OVERFLOW_ELLIPSIS = "pt-text-overflow-ellipsis";

// components
...

@blueprint-bot
Copy link

Add a new section for text utilities

Preview: docs
Coverage: core | datetime

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Yay! LGTM.

@adidahiya adidahiya merged commit 4c588e7 into master Feb 17, 2017
@adidahiya adidahiya deleted the tgreen/add-classes branch February 17, 2017 21:47
@tgreenwatts
Copy link
Contributor Author

Thanks @adidahiya for merging while I was ooto!

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.

None yet

7 participants