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

Shawnhill121 (feature)half empty heart #247

Conversation

shawnhill121
Copy link

Description
This adds Half-Empty Heart Icons to the existing Icon set, and also to the Index.html page and Storybook Icons.

Compatibility
This adds a new half empty heart icon, based on the existing one. This does not alter any existing icons.

Caveats
None.

Adds Half empty heart based on existing heart icon, also adds half empty heart to Storybook for testing and index.html for preview.
@guastallaigor guastallaigor changed the base branch from master to develop January 8, 2019 09:32
@guastallaigor guastallaigor added the enhancement New feature or request label Jan 8, 2019
@guastallaigor guastallaigor requested a review from a team January 8, 2019 09:32
Copy link
Member

@abdallahalsamman abdallahalsamman left a comment

Choose a reason for hiding this comment

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

Files paths are wrong for some reason.

Can you please fix the paths:
icons.stories.js to docs/icons.stories.js
and the scss files in the right directory scss

Copy link
Member

@abdallahalsamman abdallahalsamman left a comment

Choose a reason for hiding this comment

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

A couple more changes and we should be good 👍

@@ -38,6 +39,9 @@ stories.add('icon', () => {
'is-small': 'is-small',
'is-medium': 'is-medium',
'is-large': 'is-large',
'is-half': 'is-half',
Copy link
Member

Choose a reason for hiding this comment

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

thanks for your PR 😍

42-43 only apply to the heart icon, so they're more an icon than a variation.

Can we add is-half and is-empty after line 20 as normal icons?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I'll take care of that. Sorry it's been a little jacked up, I'm used to using git in TFS for version control.

index.html Outdated
@@ -165,6 +165,16 @@ <h2 class="title">Form</h2>
</div>
</div>
</div>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

I think this change doesn't belong to this PR, right?

Can we remove it please?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I removed it from this one.

Removes the File Input code from the previous PR.
Moves is-half and is-empty to from size selection to Icons section as they only affect the heart icon.
@BcRikko BcRikko added the waiting - contributor Waiting for the contributor to address some situations label Jan 17, 2019
@guastallaigor guastallaigor mentioned this pull request Jan 18, 2019
10 tasks
@guastallaigor
Copy link
Member

Closing this because of #387.

@guastallaigor guastallaigor removed the waiting - contributor Waiting for the contributor to address some situations label Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants