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

Add Flag component #322

Merged
merged 5 commits into from
Jul 5, 2016
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jul 5, 2016

API discussion

This component is pretty simple. There is only one changable property. I propose use name property as in Icon #279.

<i class="ae flag"></i>
<i class="france flag"></i>
<i class="myanmar flag"></i>
<Flag name='ae' />
<Flag name='france' />
<Flag name='myanmar' />

Feedback welcome 😄

@layershifter
Copy link
Member Author

Fixes #177

@layershifter layershifter mentioned this pull request Jul 5, 2016
@codecov-io
Copy link

codecov-io commented Jul 5, 2016

Current coverage is 89.01%

No coverage report found for master at 9cf78bd.

Powered by Codecov. Last updated by 9cf78bd...4e078f7

'flag',
)

return (<i className={classes} {...rest} />)
Copy link
Member

Choose a reason for hiding this comment

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

Parens are only required for multi line JSX. They can be removed here.

@levithomason
Copy link
Member

Looks good to me 👍 Let's add the flag names, ISO codes, and aliases in the prop validation for name to finish it off.

@layershifter
Copy link
Member Author

Let's add the flag names, ISO codes, and aliases in the prop validation for name to finish it off.

Do you mean something like this?

const names = [
  'andorra',
  'ad',
  ...,
  'burma',
  'mm',
  'myanmar'
];

Flag._meta = {
  props: {
    name: names
  }
}

Flag.propTypes = {
 size: PropTypes.oneOf(Flag._meta.props.name)
}

@levithomason
Copy link
Member

Yep, that will do it. As far as syntax, you can list them out horizontally since there will be many. We allow 120 columns per line.

@levithomason
Copy link
Member

Great, lmk if there is anything else you'd like to do here. Otherwise, I'll merge this once tests pass.

@levithomason
Copy link
Member

Oh, almost forgot. Go ahead and check it off in the README.md under support :)

@layershifter
Copy link
Member Author

Updated branch and README 😄

.should.have.tagName('i')
})

it('passes property "name" to class', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this earlier, this test should be replaced with the common tests for "value only" props:

common.propValueOnlyToClassName(Flag, 'name')

This test ensures the prop is defined in _meta, is added to className when it should be, and is not added when it shouldn't be.

@levithomason levithomason merged commit d6c69d9 into Semantic-Org:master Jul 5, 2016
@levithomason
Copy link
Member

Thanks for the component!

jhchill666 pushed a commit to jhchill666/stardust that referenced this pull request Jul 19, 2016
* Flag component

* Flag component Semantic-Org#322

* README.md update Semantic-Org#322

* Flag Component test update Semantic-Org#321
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.

3 participants