fix(List): Update to v1 API #551

layershifter · 2016-09-26T18:08:25Z

This PR updates List component to v1 API.

layershifter · 2016-09-26T18:34:03Z

docs/app/Examples/elements/List/Types/ListDivided.js

+        </List.Content>
+      </List.Item>
+      <List.Item>
+        <List.Icon name='github' size='large' verticalAlign='middle' />


We have there hateful situation, Icon doesn't have prop verticalAlign but .list .icon has. I don't like idea with List.Icon, may be something better?

For clarity, we're talking about this:

Oh boy :/ The proper extrapolation of our API is to add List.Icon. However, odds of users using this over Icon are slim. Especially considering all other components use the Icon directly.

I think we add verticalAlign to the Icon itself. I realize that prop would only be valid when used in a List, but it seems to make the most sense for users.

Any other wise ideas @jcarbo?

I think List.Icon might actually be the way to go. It would just be a wrapper around Icon that handles the verticalAlign prop:

function ListIcon(props) { const { verticalAlign, className } = props const classes = cx( useValueAndKey(verticalAlign, 'verticalAlign'), className ) const rest = getUnhandledProps(ListIcon, props) return <Icon {...rest} className={classes} /> }

In all likelihood, if I were using a List I'd either be:

using the icon prop as a shorthand, so it wouldn't matter whether it's Icon or List.Icon under the hood.

looking at the example page for List to see how to structure it and would see the List.Icon subcomponent there.

I could also see just building it into the Icon class with a comment noting that it's a specific use-case, although I really wish there were at least one other use-case for that.

Agreed. I was on the fence as well, but after sleeping on it and hearing this I think we should stick with the API pattern we've chosen rather than make an exception.

@layershifter let's keep it consistent and go with List.Icon. Thanks for the input guys.

codecov-io · 2016-09-26T18:49:17Z

Current coverage is 99.61% (diff: 100%)

Merging #551 into master will increase coverage by 0.12%

@@             master       #551   diff @@
==========================================
  Files           112        117     +5   
  Lines          1767       1807    +40   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1758       1800    +42   
+ Misses            9          7     -2   
  Partials          0          0

Powered by Codecov. Last update a89545d...ff77fc0

layershifter · 2016-09-29T19:23:12Z

src/elements/List/ListItem.js

+  const rest = getUnhandledProps(ListItem, props)
+  const valueProp = ElementType === 'li' ? { value } : { dataValue: value }
+
+  return <ElementType {...rest} {...valueProp} className={classes}>{children}</ElementType>


Comrades, what you think about this (valueProp)? May be more elegant solution possible there?

I'm talking about this:

<li value="*">User Benefits</li>

You can also manually specify a value for an ordered list using data-value on a div, or value on an li

I don't see any React warnings when adding value to an <li>. Any reason not to with this?

<List.Item value='*' /> //=> <li value="*"></li>

If there is an issue, one alternative could be to use bullet:

<List.Item bullet='#' /> //=> <li value="#"></li>

Though, I'd lean toward SUI parity if we can.

I'm talking about this transform, it looks not very clean (valueProp spreading).

<List.Item value='*' /> //=> <div data-value="*"></div> <List.Item as='li' value='*' /> //=> <li value="*"></li>

I see, I believe the conditional you have now is acceptable. It is the same approach I would take as a first try.

layershifter · 2016-09-29T20:24:13Z

src/elements/List/ListList.js

+  const ElementType = getElementType(ListList, props)
+  const classes = cx(
+    className,
+    useKeyOnly(ElementType !== 'ul' && ElementType !== 'ol', 'list'),


Another evil place with class handling, see SUI docs about ul & ol.
Combo like Menu.Menu is there, too :-\

<List> <List.Item> Item <List.List> <List.Item>Sub item</List.Item> </List.List> </List.Item> </List> // ... // <div class="ui list"> // <div class="item"> // <div class="list">...</div> // </div> // </div> // ...

<List as='ul'> <List.Item as='li'> Item <List.List as='ul'> <List.Item as='li'>Sub item</List.Item> </List.List> </List.Item> </List> // ... // <ul class="ui list"> // <li> // <ul>...</ul> // </li> // </ul> // ...

Per the docs the "list" class may be omitted "as a convenience" when using an "ol" or "ul" tag. Though, I don't think it is required to be omitted. We can likely always have the "list" class present.

I suspect we can also have the "item" class on all items as well. Let's try always having the classes first. That way, the components are more simple.

Per the docs the "list" class may be omitted "as a convenience" when using an "ol" or "ul" tag.

Sad, but it must be omitted.

Without classes:

With classes:

Oh boy, well then. I think the logic you originally had there is just the way it has to be.

layershifter · 2016-09-30T15:27:01Z

Ahoy, docs are fully there 🚢

Working on tests.

levithomason · 2016-09-30T15:28:49Z

Awesome, pulling and trying them out. Will let you know.

levithomason

Once again, thanks for the massive amount of work here. Especially for keeping up to date on all the latest decisions and incorporating them here. It is very much appreciated!

I've left a few comments. I'll update common tests as well, then we should update this branch to have the latest.

levithomason · 2016-09-30T15:32:26Z

src/elements/List/List.js

-      </ElementType>
-    )
-  }
+function List(props) {


Let's add the component description from the SUI docs as well.

levithomason · 2016-09-30T15:35:47Z

docs/app/Examples/elements/List/Types/index.js

+    />
+    <ComponentExample examplePath='elements/List/Types/ListExampleBulletedSimple'>
+      <Message info>
+        For convenience, a simple bulleted list can also use <code>ul</code> tag.


I think this message is only applicable to SUI core where the user defines HTML/CSS. Since, in Semantic-UI-React the classNames are hidden from the user. Let's go ahead and remove this.

levithomason · 2016-09-30T15:37:44Z

docs/app/Examples/elements/List/Types/index.js

+    />
+    <ComponentExample examplePath='elements/List/Types/ListExampleOrderedSimple'>
+      <Message info>
+        You can also manually specify a value for an ordered list using <code>value</code>.


This message belongs on the next example.

levithomason · 2016-09-30T15:40:29Z

docs/app/Examples/elements/List/Variations/ListExampleHorizontalNumbered.js

+import React from 'react'
+import { Image, List } from 'stardust'
+
+const ListExampleHorizontalNumbered = () => (


How about we rename this ListExampleHorizontalOrdered to keep with the prop name?

levithomason · 2016-09-30T15:44:21Z

src/elements/List/List.js

+  name: 'List',
+  type: META.TYPES.ELEMENT,
+  props: {
+    floated: ['left', 'right'],


Can use SUI.FLOATS here for the values.

levithomason · 2016-09-30T15:44:27Z

src/elements/List/ListContent.js

+  parent: 'List',
+  type: META.TYPES.ELEMENT,
+  props: {
+    floated: ['left', 'right'],


Can use SUI.FLOATS here for the values.

levithomason · 2016-09-30T15:45:30Z

src/elements/List/ListItem.js

+    useKeyOnly(active, 'active'),
+    useKeyOnly(disabled, 'disabled'),
+    className,
+    useKeyOnly(ElementType !== 'li', 'item'),


Can we move this up so the user's className is added very last?

levithomason · 2016-09-30T15:53:09Z

src/elements/index.js

@@ -24,6 +24,8 @@ export { default as Input } from './Input/Input'
 export { default as Label } from './Label/Label'
 export { default as LabelDetail } from './Label/LabelDetail'

+// TODO: Export `List`


Looks like this is done now, but we should export the rest of the List sub components as well. I'll add a common test that ensures all components are exported at the top level as well.

layershifter · 2016-09-30T16:09:54Z

I've fixed comments and added tests.

There is one failed test on Icon, could you please fix it? I have to move away for a few hours 😢
After fixes, we're ready to go as I think 👍

…into feat/list

layershifter · 2016-10-01T08:39:47Z

Updated to latest master and fixed test. @levithomason your move 👍

levithomason · 2016-10-01T19:29:21Z

I'll release this later today, thanks much!

levithomason · 2016-10-01T23:29:08Z

Released in stardust@0.49.0

layershifter mentioned this pull request Sep 26, 2016

feat(List): update to v1 API #311

Closed

levithomason mentioned this pull request Sep 26, 2016

Update legacy component APIs to the v1 API #269

Closed

14 tasks

levithomason changed the title ~~fix(Label): Update to v1 API~~ fix(List): Update to v1 API Sep 26, 2016

levithomason added the enhancement label Sep 26, 2016

layershifter commented Sep 26, 2016

View reviewed changes

levithomason mentioned this pull request Sep 29, 2016

refactor(docs): rename file name to be the same as class name, etc #564

Closed

33 tasks

layershifter and others added 4 commits September 29, 2016 17:24

fix(Label): Update to v1 API

920eda2

fix(Label): Update to v1 API

177a811

fix(routes): remove legacy list route

6f27d83

feat(List): More docs

a7a817c

layershifter force-pushed the feat/list branch from 2a0c5a7 to a7a817c Compare September 29, 2016 14:26

feat(List): More docs

47c004c

layershifter commented Sep 29, 2016

View reviewed changes

Alexander Fedyashov added 3 commits September 30, 2016 10:56

feat(List): More docs

8180c82

fix(List): Update List usage

a735db0

feat(List): More docs

b95630e

levithomason requested changes Sep 30, 2016

View reviewed changes

Alexander Fedyashov added 2 commits September 30, 2016 19:00

feat(List): add tests

013ce95

fix(List): fix review issues

008b764

layershifter added 2 commits October 1, 2016 10:07

Merge branch 'master' of https://github.com/TechnologyAdvice/stardust …

502f4ac

…into feat/list

fix(List): update test

ff77fc0

layershifter added the ready for review label Oct 1, 2016

layershifter mentioned this pull request Oct 1, 2016

feat(Label): add LabelGroup and missing docs #576

Merged

levithomason merged commit 8dd3e11 into master Oct 1, 2016

levithomason deleted the feat/list branch October 1, 2016 23:24

jeffcarbs removed the ready for review label Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(List): Update to v1 API #551

fix(List): Update to v1 API #551

layershifter commented Sep 26, 2016 •

edited

Loading

layershifter Sep 26, 2016

levithomason Sep 26, 2016

jeffcarbs Sep 27, 2016

levithomason Sep 27, 2016

codecov-io commented Sep 26, 2016 •

edited

Loading

layershifter Sep 29, 2016 •

edited

Loading

levithomason Sep 29, 2016 •

edited

Loading

layershifter Sep 29, 2016 •

edited

Loading

levithomason Sep 30, 2016

layershifter Sep 29, 2016

levithomason Sep 30, 2016

layershifter Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016

layershifter commented Sep 30, 2016

levithomason commented Sep 30, 2016

levithomason left a comment

levithomason Sep 30, 2016

levithomason Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

layershifter commented Sep 30, 2016

layershifter commented Oct 1, 2016

levithomason commented Oct 1, 2016

levithomason commented Oct 1, 2016

fix(List): Update to v1 API #551

fix(List): Update to v1 API #551

Conversation

layershifter commented Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 26, 2016 • edited Loading

Current coverage is 99.61% (diff: 100%)

layershifter Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Sep 30, 2016

levithomason commented Sep 30, 2016

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Sep 30, 2016

layershifter commented Oct 1, 2016

levithomason commented Oct 1, 2016

levithomason commented Oct 1, 2016

layershifter commented Sep 26, 2016 •

edited

Loading

codecov-io commented Sep 26, 2016 •

edited

Loading

layershifter Sep 29, 2016 •

edited

Loading

levithomason Sep 29, 2016 •

edited

Loading

layershifter Sep 29, 2016 •

edited

Loading

layershifter Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading