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

feat(chip): add / modify chip component #181

Merged
merged 14 commits into from
Jul 25, 2019
Merged

Conversation

gmharrison
Copy link
Contributor

@gmharrison gmharrison commented Jul 23, 2019

A continuation of #10 that adds icon support for chips

https://cl.ly/554d75534195

@gmharrison gmharrison marked this pull request as ready for review July 23, 2019 21:36
@gmharrison gmharrison requested a review from a team as a code owner July 23, 2019 21:36
Copy link
Contributor

@adamraider adamraider left a comment

Choose a reason for hiding this comment

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

Looks like the setState method doesnt have coverage around it, I would just delete it unless you think we need it for something

docs/html/chip/chip--with-icon.html Outdated Show resolved Hide resolved
packages/core/src/components/chip/_chip.scss Outdated Show resolved Hide resolved
packages/core/src/components/chip/_chip.scss Outdated Show resolved Hide resolved
packages/core/stories/chip.stories.js Outdated Show resolved Hide resolved
@adamraider
Copy link
Contributor

Should probably add keyboard navigation (make sure focus states work and ability to toggle with space bar)

import html from 'nanohtml';
import Chip from '../../src/components/chip';

function triggerEvent(el, type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cant remember if this is used elsewhere but we maaaay want to abstract into a util, definitely not blocking on this

@gmharrison
Copy link
Contributor Author

@adamraider added keyboard nav support! thanks for catching that

Copy link

@cmugla cmugla left a comment

Choose a reason for hiding this comment

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

AMAZING

}

_bindEventListeners() {
this._root.addEventListener('mousedown', this.onClick);
Copy link

Choose a reason for hiding this comment

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

this came up for another component I can't quite remember, but should we add this for touchstart as well? Did you test on your phone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch! it looks like both the touch events and click events retain hover styling when clicking on mobile. so i added the hover style to non mobile screens only.

here's a lil demo https://cl.ly/16bf6c68733d

</div>
</div>
);
});
storiesOf('Chip', module).add('micro', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can chain these .add statements

Copy link
Contributor

@adamraider adamraider left a comment

Choose a reason for hiding this comment

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

niiiiiiiice

@adamraider adamraider merged commit 5784303 into master Jul 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the gh/chips-continued branch July 25, 2019 19:11
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.

3 participants