feat(List): update to v1 API #311

joshuazmiller · 2016-06-28T18:57:50Z

Moved to #551

I don't think this is a complete solution. I see in the doc examples folder for List that we have a range of types and variations which I have yet to implement. If I could get some friendly feedback and pointers that'd be great especially since this is my first public pull request! Thank you.

First attempt at progress.

codecov-io · 2016-06-28T20:25:26Z

Current coverage is 88.81% (diff: 78.57%)

Merging #311 into master will increase coverage by 0.02%

@@             master       #311   diff @@
==========================================
  Files            66         68     +2   
  Lines           892        894     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            792        794     +2   
  Misses          100        100          
  Partials          0          0

Powered by Codecov. Last update 51d5f9a...4e7f6c9

levithomason · 2016-06-28T20:34:57Z

src/elements/List/List.js

-      </div>
-    )
-  }
+function _List(props) {


Since this is your first PR, I'll do a little extra explaining 😉 This is an inline comment, you can leave these by clicking a line number in the diff (use the File tab).

When a comment is left on a specific line like this, it is because we want to discuss the code on this line or just surrounding it. Here goes.

Underscore names are reserved for private components and variables. Let's rename this to List instead.

-function _List(props) { +function List(props) {

Why does _Header.js have an underscore? Is it private?

Indeed. It is a base header component to be reused among the various header components.

On a side note, the header subcomponents do not really follow our API pattern and should be rethought.

…chine and also all the examples appear to work.

levithomason · 2016-06-28T20:48:57Z

src/elements/List/List.js

+  const rest = getUnhandledProps(_List, props)
+
+  return (
+    <ListComponent className={classes} {...rest}>


Since the ListComponent never changes, let's use a div inline here instead of creating the assignment:

-const ListComponent = 'div' const rest = getUnhandledProps(List, props) return ( - <ListComponent className={classes} {...rest}> + <div className={classes} {...rest}> {children} - </ListComponent> + </div>

levithomason · 2016-06-28T21:00:17Z

The next big step for this component is to turn the supported SUI HTML classes into a React Component API. I would reference the work done in #268 which updated the Divider to our v1 API.

This is mostly centered around creating declarative props for component features. Then, using our propUtils methods (useKey, useValueAndKey, etc) to build up the className. Again, I'd reference the Divider PR #268.

We have an open issue to add documentation around this process. I'll go ahead and start that right now.

joshuazmiller · 2016-06-29T02:02:01Z

Please let me know if this last commit was the direction you wanted to take. @levithomason

This reverts commit cd7171d.

levithomason · 2016-06-29T15:48:03Z

test/specs/elements/List/List-test.js

+      .should.have.tagName('div')
+  })
+
+  it('adds the "list" class', () => {


This test is covered by the common.isConformant() test, all components are required to have their component name as a className. Let's delete this test.

I'm almost done with the docs for writing components, though it looks like you've just about got it down!

levithomason · 2016-06-30T05:58:32Z

src/elements/List/List.js

+    size,
+    useValueAndKey(aligned, 'aligned'),
+    useKeyOnly(bulleted, 'bulleted'),
+    useKeyOnly(link, 'link'),


When the link class is used, the ListItems need to use an a tag:

<div class="ui link list"> <div class="active item">Home</div> <a class="item">About</a> <a class="item">Jobs</a> <a class="item">Team</a> </div>

I've considered adding a component prop to every component, which would allow setting the component to render as. In this case, an a. When a List has link set, we could map all the ListItem children and set it like so <List.Item component='a' />.

Note the active item always uses a div. So, a <List.Item active /> should always render a div.

Thoughts?

I like this idea. However, which other use cases would this come in handy? If we can't identify other use cases then is there a way to do away with component and make this the default for non-active link-list items? I also worry that the user may leave out component when it is required or use it when it shouldn't be used. It just has the potential to lead to more bugs and confusion for end-users.

For the latest on component augmentation (component/elementType/as prop) see https://github.com/TechnologyAdvice/stardust/issues/403.

For more on the link prop and rendering as an a tag see https://github.com/TechnologyAdvice/stardust/pull/382#discussion_r75187539.

levithomason · 2016-06-30T07:24:36Z

List API Proposal

The updates made to the <List /> are looking good so I will focus only on the ListItem here. Items in SUI are a sore spot, your feedback and alternate ideas are much appreciated.

ListItem

The parent ui list was left out of all examples below for brevity.

The current ListItem supports the various list component parts via props (icon, image, header, and description). Depending on the props used, the proper markup is generated. This includes the sometimes necessary content div which wraps content adjacent to an icon or image. See the current source for the ListItem.

In order to support our latest guidelines for markup, we need to consider using sub components for these component parts. Here's why.

Props for component parts

The advantage to using props is that the JSX is minimal:

<List.Item icon='user' description='John Doe' />

The downside here is that any list item markup beyond that of plain text is awkward to work with. What if you want a header link with an href, a data attribute on your icon, or p tags in your description? You have to construct these outside the ListItem then pass them in as props.

Sub components for component parts

The advantage to using sub components is that it is flexible. This example adds all the difficult markup I just mentioned above:

<List.Item>
  <Icon name='user' data-foo='bar'>
  <List.Content>
    <List.Header href='johndoe.com'>
      John Doe
    </List.Header>
    <List.Description>
      <p>John Doe is <b>so</b> awesome.</p>
    </List.Description>
  </List.Content>
</List.Item>

The downside being that it is very verbose. However, if you need flexibility then this is acceptable.

Supporting both

I'd like to support both of these. This should be rather simple, actually.

If you have simple markup you can provide icon, image, header, or description props and we'll construct the markup for you. No children are allowed. This is what we do now, except we don't disallow children.

If you have intricate markup you may use children and construct it yourself. No icon, image, header, or description props are allowed.

We have a customPropType called mutuallyExclusive. It prevents mutually exclusive props from being used together. We can make the icon, image, header, and description props mutually exclusive with children.

The following examples show both APIs I'm proposing.

Simple Item

<List.Item description='Apples' />
// or
<List.Item>Apples</List.Item>

<div class="item">Apples</div>

Icon or Image

Markup adjacent to an image or icon must be placed in content.

<List.Item icon='user' description='Semantic UI' />
// or
<List.Item>
  <List.ItemIcon name='user' />
  <List.ItemContent>
    Semantic UI
  </List.ItemContent>
</List.Item>

<div class="item">
  <i class="users icon"></i>
  <div class="content">
    Semantic UI
  </div>
</div>

Header & Description

Since the description here is more than just plain text, the prop API is not preferred. I've shown it anyway.

const description = <span>Click a link in our <a>description</a>.</span>
<List.Item header='Header' description={description} />

// or

<List.Item>
  <List.ItemHeader>
    Header
  </List.ItemHeader>
  <List.ItemDescription>
    Click a link in our <a>description</a>.
  </List.ItemDescription>
</List.Item>

<div class="item">
  <a class="header">Header</a>
  <div class="description">
    Click a link in our <a>description</a>.
  </div>
</div>

Image, Header & Description

The example here assumes the avatar class should be added to the image when the image prop is used. Again, since the description contains more than plain the text, the props API is not ideal here. It is shown anyway.

const description = (
  <span>Last seen watching <a><b>Arrested Development</b></a> just now.</span>
)
<List.Item
  image='daniel.jpg'
  header='Daniel Louise'
  description={description}
/>

// or

<List.Item>
  <List.ItemImage src='daniel.jpg' />
  <List.ItemHeader>Daniel Louise</List.ItemHeader>
  <List.ItemDescription>
    Last seen watching <a><b>Arrested Development</b></a> just now.
  </List.ItemDescription>
</List.Item>

<div class="ui list">
  <div class="item">
    <img class="ui avatar image" src="daniel.jpg">
    <div class="content">
      <a class="header">Daniel Louise</a>
      <div class="description">Last seen watching <a><b>Arrested Development</b></a> just now.</div>
    </div>
  </div>
</div>

You can see the difficulties in using the props API for markup. Also, the verboseness but flexibility of the sub component API. Looking forward to your thoughts.

joshuazmiller · 2016-06-30T08:50:22Z

@levithomason I like this proposal very much. I think the cleanliness of sub-components makes the API more flexible and extensible for end users. I believe there will be ample instances where a user will want to enter a long description, and perhaps even include other components in a description. Passing everything through props alone would be very restrictive and cumbersome in that regard. That said, providing the shorthand format too is an excellent alternative for those opting out of unnecessary verbosity in their code. I think this would be a great step forward for maximizing the usability of our list components.

joshuazmiller · 2016-07-04T20:04:32Z

Although I can create ListItem, ListHeader, etc. as subcomponents in the same folder as List (as seems to be described in the doc) this will result in a lot of duplication esp. with the Header component which has 9 files by itself (H1, H2, etc. Subheader). Do you still suggest these as duplicate components under the List directory?

joshuazmiller · 2016-07-04T21:43:22Z

Take my last commit with a grain of salt. I'm not positive I took this in the direction you had in mind. The new commit does let you create list items both via sub-components and non-sub which was the primary goal. I also create the content and description elements which I'm not positive you wanted separated as reusable elements; it just seemed in line with the current style, but maybe I'm mistaken. The ItemHeader file is the file I have the biggest questions about both in placement of the file and the overall necessity as a separate file. Additionally, perhaps you were thinking that everything should be named Item* but I think that would lead to a lot of code duplication so I moved my initial commit against that. I'm looking forward to your feedback on this!

levithomason · 2016-07-04T21:47:05Z

Note the List header "component part" does not support different h* tags nor subheader as the SUI "component" does.

SUI "component parts" often do not support nearly as many feature as their "component" counter parts. Example, a list "header" vs a "ui header". The same is true for the "ui item" vs the list "item".

Though some components share the class "item" or "header", the supported features and markup are often entirely different. Because of this, I don't think we should reuse them between components.

All said, I'm completely open to better proposals.

joshuazmiller · 2016-07-04T21:51:28Z

docs/app/Examples/elements/List/Content/ListHeaderExample.js

@@ -8,6 +8,16 @@ export default class ListHeaderExample extends Component {
        <List.Item header='Chapter 1' description='The chapter in which we meet the characters' />
        <List.Item header='Chapter 2' description='The chapter in which the bad guy is introduced' />
        <List.Item header='Chapter 3' description='Spoiler alert: The chapter in which the good guy wins!' />
+        <List.Item>
+          <List.ItemContent>


When there is no image/icon do we want to require ItemContent? The current ListItem file suggests no.
ListItem.js: if (media) content = <div className='content'>{content}</div>

Correct, no content when there is no icon/image. Reference the SUI docs for the exact markup.

http://semantic-ui.com/elements/list

In the case of the Item view, adding content with no icon/image will actually break the vertically aligned variations.

joshuazmiller · 2016-07-04T22:05:01Z

In response to your previous comment, what about icon and image? Do you feel those can be reused? Also, do you think placing header into a separate ItemHeader.js file is the way to go, and if it should be its own file, do you think it should be placed under the List directory or the views/Item directory? Thanks.

levithomason · 2016-07-05T00:51:56Z

In response to your previous comment, what about icon and image? Do you feel those can be reused?

The SUI List docs appear to use the same markup as the Icon / Image components, so yes.

Also, do you think placing header into a separate ItemHeader.js file is the way to go, and if it should be its own file, do you think it should be placed under the List directory or the views/Item directory?

Yes, separate file. We have a lint rule that enforces this. It is important to keep one component per file for doc generation as well.

All List subcomponents should go in the List directory. I can see the reason for asking this since the name is ItemHeader. We may end up renaming things a bit once this is all laid out.

Please forgive the brevity and any typos as I am on mobile. Thanks for all the work so far!

joshuazmiller · 2016-07-06T05:12:07Z

Do we put separate examples of using the component markup in the docs?

levithomason · 2016-07-06T05:20:33Z

Do we put separate examples of using the component markup in the docs?

Which markup are you referring to here?

joshuazmiller · 2016-07-06T06:01:22Z

Which markup are you referring to here?

Examples of using the sub-component markup versus using regular props.

levithomason · 2016-07-06T06:08:44Z

Ah, yes please. You can reference the Accordion docs where I documented the same type of API options. I've given examples of both the sub component markup and the props for markup. I've also included docs in the props descriptions for this.

levithomason · 2016-07-07T07:45:15Z

Regarding reuse of "component parts", I did some more thorough analysis of the header component part and here is what I'm thinking.

Header "Component Parts"

Looking at every component that supports the header part (not the ui header component), what is the common denominator and how can we effectively reuse this? I've also linked the SUI docs for each.

Card Header

wrapped in content when sibling to icon

header class
can be an a tag

Dropdown Header

Does not directly support a header only by the fact that its component parts do.

as .dropdown > .menu > .header
as .dropdown > .message > .header

Item Header

always wrapped in content, even with no image sibling

header class
can be an a tag

List Header

wrapped in content when sibling to icon/image

header class
can be an a tag

Menu Header

never wrapped in content, never has sibling icon/image

header class (in an item)
header item class (as an item)
can be an a tag, not documented but valid (header as an link item)

Message Header

wrapped in content when sibling to icon/image

header class
a tag not documented, but valid

Modal Header

header class
a tag not documented, but does not pose an issue

Conclusion

Common: header class, can be an a tag

Unique: item class (Menu)

Proposal

How about we create a src/parts directory and begin reusable "component parts" there? The "component parts" will include only common denominator functionality and props. Individual components can then extend as needed. Example:

// src/parts/HeaderPart.js

function HeaderPart() {
  // support "header" class, href/onClick, `a` tag
}

// src/collections/Menu/MenuHeader.js
import HeaderPart from 'src/parts/HeaderPart'

function MenuHeader() {
  // render HeaderPart adding the "item" class when necessary
}

We would then do the same for content, meta, description, extra, etc. If this seems sane, I can start on these "parts" and get the baseline going.

Thoughts?

/cc @jamiehill would love your input here as well.

jhchill666 · 2016-07-07T08:11:34Z

I agree that this needs standardising. My initial thinking was that the extra functionality required by varying components should be baked into Header it's self, but on second thoughts would pollute the logic trying to cater for different edge cases.

There seems to be some merit in maintaining the abstraction of using Header though. Would it not be convenient to be able to utilize the Header sub comps:

<Card>
   <Card.Header.H1>Large Card Header</Card.Header.H1>
</Card>

If we abstract the core header logic into a separate component part, we can indeed then still composite variations, for prescribing components, using additional header component parts. I think the key here is that we don't want to expose non-pertinent props on Header, but still use a decorated version of it in other components, while keeping the api as standardized as possible?

levithomason · 2016-07-07T17:58:46Z

I'm with you. There's just too much variation between "components" and "component parts" to standardize completely.

I've moved this conversation to "RFC: Standardize Component Parts" #325. You'll find a more thorough analysis and updated proposal there. Feedback welcome.

levithomason · 2016-07-18T17:36:53Z

test/specs/elements/List/List-test.js

+   * The correct test for 'relaxed' would be something that
+   * tests for useKeyOrValueAndKey which doesn't seem to be
+   * available.
+   */


It sounds like you are looking for common.propKeyOrValueToClassName. Note there is a common prop to className test for every propUtil.

https://github.com/TechnologyAdvice/stardust/blob/master/CONTRIBUTING.md#testing-classname

levithomason · 2016-08-18T16:30:20Z

@joshuazmiller it looks like this PR has gotten stale. If you add me as a collaborator to your fork, I can help update it and get it merged. LMK what your plans are.

layershifter · 2016-09-26T17:36:25Z

Are there any activity? I can start over it on the weekend.

levithomason · 2016-09-26T17:38:01Z

This one is open game IMO. It might be worth starting from scratch and just copying over the relevant files. The merge conflicts would be crazy otherwise.

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

I'm opened #551, seems we can close this PR.

joshuazmiller added 3 commits June 28, 2016 11:37

Convert List to v1 API

6197943

First attempt at progress.

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

6c7a3d6

Merge branch 'master' into master

959aff4

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

e1f7042

levithomason reviewed Jun 28, 2016
View reviewed changes

joshuazmiller added 2 commits June 28, 2016 13:43

Finished refactoring List.js for v1. It works without errors on my ma…

0906e78

…chine and also all the examples appear to work.

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

5d071df

levithomason reviewed Jun 28, 2016
View reviewed changes

joshuazmiller added 3 commits June 28, 2016 13:49

Renamed _List to List

fdd3113

Got rid of ListComponent var and inlined 'div'

268c3c8

Updated List.js to get rid of 3 Linting errors.

1e86005

joshuazmiller added 2 commits June 28, 2016 18:57

List: replaced classNames with declarative props

7e24d28

Merge branch 'master' into master

1bd5965

joshuazmiller added 6 commits June 28, 2016 19:49

List: added tests

681fa30

Merge remote-tracking branch 'origin/master'

057be3a

List: refined tests

d83a3c0

List: corrected lint

202a8b9

Supporting env var assignments on Windows

cd7171d

Revert "Supporting env var assignments on Windows"

bf48ba3

This reverts commit cd7171d.

levithomason reviewed Jun 29, 2016
View reviewed changes

levithomason mentioned this pull request Jun 29, 2016

Update legacy component APIs to the v1 API #269

Closed

14 tasks

levithomason reviewed Jun 30, 2016
View reviewed changes

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

f9ddcd0

Fixing small things based on pull-request Semantic-Org#311

8245887

feat: add ability to create list items via subcomponents

4e7f6c9

joshuazmiller reviewed Jul 4, 2016
View reviewed changes

levithomason added the enhancement label Jul 5, 2016

levithomason mentioned this pull request Jul 7, 2016

RFC: Standardize Component Parts #325

Closed

levithomason reviewed Jul 18, 2016
View reviewed changes

levithomason changed the title ~~Update List to v1 API~~ List: update to v1 API Aug 18, 2016

levithomason changed the title ~~List: update to v1 API~~ feat(List): update to v1 API Sep 19, 2016

levithomason closed this Sep 26, 2016

levithomason added invalid and removed enhancement labels Sep 26, 2016

levithomason mentioned this pull request Sep 28, 2016

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

Closed

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(List): update to v1 API #311

feat(List): update to v1 API #311

joshuazmiller commented Jun 28, 2016 •

edited by levithomason

Loading

codecov-io commented Jun 28, 2016 •

edited

Loading

levithomason Jun 28, 2016

joshuazmiller Jun 28, 2016

levithomason Jun 30, 2016

levithomason Jun 28, 2016 •

edited

Loading

levithomason commented Jun 28, 2016

joshuazmiller commented Jun 29, 2016

levithomason Jun 29, 2016

levithomason Jun 30, 2016

joshuazmiller Jun 30, 2016

levithomason Aug 18, 2016

levithomason commented Jun 30, 2016 •

edited

Loading

joshuazmiller commented Jun 30, 2016

joshuazmiller commented Jul 4, 2016

joshuazmiller commented Jul 4, 2016

levithomason commented Jul 4, 2016

joshuazmiller Jul 4, 2016 •

edited

Loading

levithomason Jul 5, 2016

joshuazmiller commented Jul 4, 2016

levithomason commented Jul 5, 2016

joshuazmiller commented Jul 6, 2016

levithomason commented Jul 6, 2016

joshuazmiller commented Jul 6, 2016

levithomason commented Jul 6, 2016

levithomason commented Jul 7, 2016

jhchill666 commented Jul 7, 2016 •

edited

Loading

levithomason commented Jul 7, 2016 •

edited

Loading

levithomason Jul 18, 2016

levithomason Jul 18, 2016

levithomason commented Aug 18, 2016

layershifter commented Sep 26, 2016

levithomason commented Sep 26, 2016

layershifter commented Sep 26, 2016

feat(List): update to v1 API #311

feat(List): update to v1 API #311

Conversation

joshuazmiller commented Jun 28, 2016 • edited by levithomason Loading

Moved to #551

codecov-io commented Jun 28, 2016 • edited Loading

Current coverage is 88.81% (diff: 78.57%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason commented Jun 28, 2016

joshuazmiller commented Jun 29, 2016

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

levithomason commented Jun 30, 2016 • edited Loading

List API Proposal

ListItem

Simple Item

Icon or Image

Header & Description

Image, Header & Description

joshuazmiller commented Jun 30, 2016

joshuazmiller commented Jul 4, 2016

joshuazmiller commented Jul 4, 2016

levithomason commented Jul 4, 2016

joshuazmiller Jul 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuazmiller commented Jul 4, 2016

levithomason commented Jul 5, 2016

joshuazmiller commented Jul 6, 2016

levithomason commented Jul 6, 2016

joshuazmiller commented Jul 6, 2016

levithomason commented Jul 6, 2016

levithomason commented Jul 7, 2016

Header "Component Parts"

Conclusion

Proposal

jhchill666 commented Jul 7, 2016 • edited Loading

levithomason commented Jul 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Aug 18, 2016

layershifter commented Sep 26, 2016

levithomason commented Sep 26, 2016

layershifter commented Sep 26, 2016

joshuazmiller commented Jun 28, 2016 •

edited by levithomason

Loading

codecov-io commented Jun 28, 2016 •

edited

Loading

levithomason Jun 28, 2016 •

edited

Loading

levithomason commented Jun 30, 2016 •

edited

Loading

joshuazmiller Jul 4, 2016 •

edited

Loading

jhchill666 commented Jul 7, 2016 •

edited

Loading

levithomason commented Jul 7, 2016 •

edited

Loading