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(Card): add link prop #1359

Merged
merged 3 commits into from
Feb 21, 2017

Conversation

BrainMaestro
Copy link
Contributor

@BrainMaestro BrainMaestro commented Feb 21, 2017

Fixes #1350

Does this need extra tests to check that the correct styles were applied to the card if it has the link prop?

@layershifter
Copy link
Member

Let's also add example for link prop like in SUI docs.

@codecov-io
Copy link

codecov-io commented Feb 21, 2017

Codecov Report

Merging #1359 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1359      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         140      140              
  Lines        2390     2392       +2     
==========================================
+ Hits         2384     2386       +2     
  Misses          6        6
Impacted Files Coverage Δ
src/views/Card/Card.js 100% <ø> (ø)
src/collections/Form/FormDropdown.js 100% <ø> (ø)
src/collections/Form/FormTextArea.js 100% <ø> (ø)
src/collections/Form/FormSelect.js 100% <ø> (ø)
src/collections/Form/FormField.js 100% <ø> (ø)
src/collections/Form/FormCheckbox.js 100% <ø> (ø)
src/collections/Form/FormButton.js 100% <ø> (ø)
src/collections/Form/FormGroup.js 100% <ø> (ø)
src/collections/Form/FormRadio.js 100% <ø> (ø)
src/collections/Form/Form.js 100% <ø> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8f826c...b11380d. Read the comment docs.

@BrainMaestro
Copy link
Contributor Author

@layershifter I added one

meta='Scientist'
description='Rick is a genius scientist whose alcoholism and reckless, nihilistic behavior are a source of concern for his family'
/>
</Card.Group>
Copy link
Member

Choose a reason for hiding this comment

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

Let's split these examples to separate files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but in the actual docs, they're next to each other. Since they're showing the same property functionality, shouldn't they be together?

Copy link
Member

Choose a reason for hiding this comment

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

You can make these appear together while still having separate example files. The advantage is that each example will have its own code editor.

To do this, create a new file and add it to the index.js in this directory just after the CardExampleLinkCard.js entry, but do not include a title nor description:

docs/app/Examples/views/Card/Content/index.js

    <ComponentExample
      title='Link'
      description='A card can be formatted to link to other content.'
      examplePath='views/Card/Content/CardExampleLinkCard'
    />
    <ComponentExample
      examplePath='views/Card/Content/CardExampleLinkCardProp'
    />

The second example will render directly below the title/description of the first example. This is common in our docs for grouping examples.

@@ -55,6 +55,9 @@ export default class Card extends Component {
/** A card can contain an Image component. */
image: customPropTypes.itemShorthand,

/** A card can be formatted to behave like a link (hover effects, pointer cursor) */
Copy link
Member

@levithomason levithomason Feb 21, 2017

Choose a reason for hiding this comment

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

Let's go with our doc site description:
A card can be formatted to link to other content.

Since, the specifics of the style can vary from theme to theme.

@BrainMaestro
Copy link
Contributor Author

I updated with your suggestions

@levithomason
Copy link
Member

Changes look great, I'll merge as soon as tests pass:

/home/ubuntu/Semantic-UI-React/docs/app/Examples/views/Card/Content/CardExampleLinkCardProp.js
  9:1  error  Line 9 exceeds the maximum line length of 120  max-len

@levithomason
Copy link
Member

I've just pushed a fix for the description prop syntax error.

@BrainMaestro
Copy link
Contributor Author

Yeah I see it. Odd, didn't show up as an error locally

@levithomason
Copy link
Member

Looks like this is a lint error, so you'd only see it by running npm run lint. We don't run lint automatically as it can take ~20 seconds to run and would hinder development workflow. We rely heavily on the PR status checks instead.

Not as intuitive though, sorry about that :)

@BrainMaestro
Copy link
Contributor Author

Oh yeah I ran the lint check locally as well and it was fine. Oh well, is this good to merge now?

@levithomason
Copy link
Member

Oh, that is odd... perhaps outdated deps? Not sure, but, we're good to go! Thanks much for the contribution, hope to have you around for some more 👍

@levithomason levithomason merged commit a93729c into Semantic-Org:master Feb 21, 2017
@BrainMaestro BrainMaestro deleted the add-link-prop-to-card branch February 21, 2017 19:12
@BrainMaestro
Copy link
Contributor Author

Thanks for being patient! I have had a great experience so far with this project, and I hope to contribute more.

@levithomason
Copy link
Member

Whoops, @layershifter we missed typings here :/

@levithomason
Copy link
Member

Released in [email protected].

harel pushed a commit to harel/Semantic-UI-React that referenced this pull request Feb 25, 2017
* feat(Card): add link prop

* docs(Card): add example for card link prop

* fix(CardExampleLinkCardProp): description prop
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.

4 participants