feat(List): Shorthand for list and list items #637

jeffcarbs · 2016-10-06T16:02:23Z

Fixes #589

This adds shorthand for list and list item. Still somewhat WIP:

Add missing test coverage
~~Add pass-through props to item-content~~ Solved a different way

Only open question is regarding documentation. How do we want to handle shorthand documentation? I added a few examples here but there's a lot of them for List so I wanted to clarify our approach before going much further. Some thoughts:

I definitely think we should write shorthand for all examples.
I think the best idea would be to just show the example once and show the shorthand side-by-side with the regular code.
- Showing the shorthand in separate section makes it less discoverable.
- Showing the same exact example twice is redundant and makes the docs hard to read.

This (more advanced shorthand documentation) may be something we leave for a separate PR, just wanted to get thoughts.

jeffcarbs · 2016-10-06T16:04:25Z

src/elements/Header/Header.js

@@ -52,10 +52,13 @@ function Header(props) {
    return <ElementType {...rest} className={classes}>{children}</ElementType>
  }

-  if ((image && typeof image !== 'boolean') || (icon && typeof icon !== 'boolean')) {


These checks weren't entirely accurate. image or icon could have been passed as empty strings (valid itemShorthand) but still wouldn't be rendered. Also, the !== boolean is captured by the shorthand factory, which ignores booleans.

Cool, I'm on board with using the return of the factory to determine the markup. Seems stronger to isolate that log in the factory's handling of the shorthand values.

levithomason · 2016-10-06T17:51:33Z

src/elements/Header/Header.js

@@ -52,10 +52,13 @@ function Header(props) {
    return <ElementType {...rest} className={classes}>{children}</ElementType>
  }

-  if ((image && typeof image !== 'boolean') || (icon && typeof icon !== 'boolean')) {
+  const iconNode = Icon.create(icon)
+  const imageNode = Image.create(image)


I've been moving these names to fooElement. We had a number of fooJSX previously. I think element is most accurate since factories return a ReactElement. LMK if you disagree on the name, but I do want to make them consistent.

levithomason · 2016-10-06T17:53:42Z

src/elements/Header/Header.js

@@ -52,10 +52,13 @@ function Header(props) {
    return <ElementType {...rest} className={classes}>{children}</ElementType>
  }

-  if ((image && typeof image !== 'boolean') || (icon && typeof icon !== 'boolean')) {


Cool, I'm on board with using the return of the factory to determine the markup. Seems stronger to isolate that log in the factory's handling of the shorthand values.

levithomason · 2016-10-06T17:54:07Z

src/elements/List/List.js

+
+  return (
+    <ElementType {...rest} className={classes}>
+      {_.map(items, (item) => ListItem.create(item))}


Missing coverage here. Codecov is still not updating PR status for some reason. Killing me...

levithomason · 2016-10-06T17:54:41Z

src/elements/List/ListItem.js

+      <ElementType {...rest} className={classes} {...valueProp}>
+        {iconNode || imageNode}
+        {(content || headerNode || descriptionNode) && (
+          <ListContent>


levithomason · 2016-10-06T17:55:16Z

src/elements/List/ListItem.js

+  header: customPropTypes.itemShorthand,
+
+  /** Shorthand for ListIcon. */
+  icon: customPropTypes.every([


What do you say we add iconShorthand and imageShorthand that can handle disallowing the each other? This is also needed in the header, perhaps elsewhere.

I think that's more of an exception, tbh. A quick search shows it's only applicable in the header and list while icon is a prop in over 20 component/subcomponents (although maybe we're not applying the disallow in some places we should be?).

In any case, the *Shorthand props includes a disallow(['children']) because that's required in all cases. This is only applicable in some cases so I think it'll make it more clear to apply the disallow within the component's prop definitions that need it.

levithomason · 2016-10-06T17:58:22Z

Agree on shorthand examples, I think there should be a shorthand example following every regular example. See our resolution on this here #564 (comment).

You can also reference the Feed docs which I think are a good example of shorthand following the regular example.

codecov-io · 2016-10-06T21:21:47Z

Current coverage is 100% (diff: 100%)

Merging #637 into master will not change coverage

@@           master   #637   diff @@
====================================
  Files         119    119          
  Lines        1890   1916    +26   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1890   1916    +26   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 978f206...48104f2

jeffcarbs · 2016-10-07T13:29:25Z

@levithomason - added missing specs. Looks like the build notification is broken for codecov: https://codecov.io/gh/Semantic-Org/Semantic-UI-React/commit/edc973113439d5a74414f62203407b711b5baea9/builds.

2016-10-07T01:25:38-04:00 notify POST /repos/Semantic-Org/Semantic-UI-React/statuses/edc973113439d5a74414f62203407b711b5baea9 bot=matheuspoleza code=404
{"message":"Not Found","documentation_url":"https://developer.github.com/v3"}

Perhaps you need to update that key in the url?

levithomason · 2016-10-08T03:18:31Z

Perhaps you need to update that key in the url?

The key is the commit sha:

For some reason, making a POST to that endpoint gives a 404 saying it can't be found. I'm betting this is due to renaming the original Semantic-UI-React repo to Semantic-UI-React-Old, then transferring stardust to Semantic-UI-React, and then deleting the original Semantic-UI-React-Old repo. GitHub sets up redirects to handle all the moving around. I'm thinking the transfer/rename/delete has confused GH or one of these services. Especially being I went back to TechnologyAdvice and created another stardust so I could redirect the old docs to the new docs. This means there is a real repo there again, so links to it should not redirect any more.

The coverage reports are uploading to Codecov from CircleCI OK, and Codecov shows the correct coverage. It also comments on the PR correctly. It just seems the /statuses endpoint is not finding the repo. I've reached out to Codecov support to see if they can help as well.

levithomason · 2016-10-08T03:30:42Z

src/elements/List/ListItem.js

+   * - a props object, it forces the presence of Item.Content and passes those
+   * props to it. If you pass a content prop within that props object, it
+   * will be treated as the sibling node to header/description.
+   */


I pushed an update to the ComponentProps component to preserve line breaks. That way this doesn't render as a single ran together line in the prop table.

levithomason · 2016-10-08T03:41:34Z

Made a fix to the simple bulleted example, it doesn't require the bulleted prop since the elements ul and li are provided. Then, I added some descriptions to the other examples. Will merge on pass.

levithomason · 2016-10-08T04:37:21Z

Released in semantic-ui-react@0.54.5

jeffcarbs commented Oct 6, 2016

View reviewed changes

levithomason requested changes Oct 6, 2016

View reviewed changes

jeffcarbs mentioned this pull request Oct 6, 2016

feat(ListItem): add shorthand #589

Closed

jeffcarbs added 2 commits October 7, 2016 00:57

feat(List): Shorthand for list and list items

5609e8d

Handle ListContent

8b2ad59

jeffcarbs force-pushed the feature/list-shorthand branch from 3e8b5a3 to 76e5136 Compare October 7, 2016 04:57

Add shorthand specs for List

edc9731

jeffcarbs force-pushed the feature/list-shorthand branch from 76e5136 to edc9731 Compare October 7, 2016 05:18

layershifter added the enhancement label Oct 7, 2016

feat(ComponentProps): render doc block line breaks

6acc9e5

levithomason reviewed Oct 8, 2016

View reviewed changes

levithomason added 3 commits October 7, 2016 20:36

docs(List): add shorthand descriptions

c29ecbc

docs(List): fix simple bulleted example

4143e15

docs(List): add simple ordered description

48104f2

levithomason approved these changes Oct 8, 2016

View reviewed changes

levithomason added the ready for review label Oct 8, 2016

levithomason merged commit 7ae2ed2 into master Oct 8, 2016

levithomason deleted the feature/list-shorthand branch October 8, 2016 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(List): Shorthand for list and list items #637

feat(List): Shorthand for list and list items #637

jeffcarbs commented Oct 6, 2016 •

edited

Loading

jeffcarbs Oct 6, 2016 •

edited

Loading

levithomason Oct 6, 2016

levithomason Oct 6, 2016

levithomason Oct 6, 2016

levithomason Oct 6, 2016

levithomason Oct 6, 2016

levithomason Oct 6, 2016 •

edited

Loading

jeffcarbs Oct 7, 2016

levithomason commented Oct 6, 2016

codecov-io commented Oct 6, 2016 •

edited

Loading

jeffcarbs commented Oct 7, 2016

levithomason commented Oct 8, 2016 •

edited

Loading

levithomason Oct 8, 2016

levithomason commented Oct 8, 2016

levithomason commented Oct 8, 2016

feat(List): Shorthand for list and list items #637

feat(List): Shorthand for list and list items #637

Conversation

jeffcarbs commented Oct 6, 2016 • edited Loading

jeffcarbs Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 6, 2016

Choose a reason for hiding this comment

levithomason Oct 6, 2016

Choose a reason for hiding this comment

levithomason Oct 6, 2016

Choose a reason for hiding this comment

levithomason Oct 6, 2016

Choose a reason for hiding this comment

levithomason Oct 6, 2016

Choose a reason for hiding this comment

levithomason Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

jeffcarbs Oct 7, 2016

Choose a reason for hiding this comment

levithomason commented Oct 6, 2016

codecov-io commented Oct 6, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

jeffcarbs commented Oct 7, 2016

levithomason commented Oct 8, 2016 • edited Loading

levithomason Oct 8, 2016

Choose a reason for hiding this comment

levithomason commented Oct 8, 2016

levithomason commented Oct 8, 2016

jeffcarbs commented Oct 6, 2016 •

edited

Loading

jeffcarbs Oct 6, 2016 •

edited

Loading

levithomason Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 6, 2016 •

edited

Loading

levithomason commented Oct 8, 2016 •

edited

Loading