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

[Icon] Fix pt-icon-bank-account className #1458

Merged
merged 1 commit into from
Aug 17, 2017
Merged

[Icon] Fix pt-icon-bank-account className #1458

merged 1 commit into from
Aug 17, 2017

Conversation

llorca
Copy link
Contributor

@llorca llorca commented Aug 16, 2017

Fixes #1455

(Technically a breaking change, but we think it'd be silly to wait for 2.0 for such a small correction)

@llorca llorca requested review from cmslewis and pkwi August 16, 2017 18:01
@blueprint-bot
Copy link

Fix pt-icon-bank-account className

Preview: documentation
Coverage: core | datetime

@cmslewis
Copy link
Contributor

@llorca wait, does this mean people had to write stuff like <a class="pt-button pt-icon-banl-account"> before now? If so, isn't this PR effectively breaking, even though it's a correction?

@llorca
Copy link
Contributor Author

llorca commented Aug 16, 2017

Yeah that's probably true, but I doubt anybody would've used it without calling out the error. In any case, that fix seems fine to me -- don't think it'd be appropriate to wait for 2.0 for such a thing?
image

@cmslewis
Copy link
Contributor

cmslewis commented Aug 16, 2017

Yeah waiting to 2.0 seems like overkill. Just wanted to call it out.

EDIT: Updated the PR description.

@cmslewis
Copy link
Contributor

Let's get one more opinion from @adidahiya or @giladgray before we merge.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

sgtm

@adidahiya adidahiya merged commit 8375de5 into master Aug 17, 2017
@adidahiya adidahiya deleted the llorca-patch-1 branch August 17, 2017 04:44
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.

4 participants