Container Component updated to v1 API #277

jhchill666 · 2016-06-24T07:18:03Z

Container Component updated to v1 API
Examples updated to match Semantic Docs
Container-test added to suite

- Container Component updated to v1 API - Examples updated to match Semantic Docs - Container-test added to suite

codecov-io · 2016-06-24T07:47:40Z

Current coverage is 86.00%

Merging #277 into master will increase coverage by 0.30%

@@             master       #277   diff @@
==========================================
  Files            62         62          
  Lines           797        800     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            683        688     +5   
+ Misses          114        112     -2   
  Partials          0          0

Powered by Codecov. Last updated by 312c234...28f6d7f

…container

levithomason · 2016-06-24T15:58:29Z

docs/app/Examples/elements/Container/Types/ContainerTextExample.js

+import React, { Component } from 'react'
+import { Container, Message } from 'stardust'
+
+export default class ContainerContainerExample extends Component {


-class ContainerContainerExample +class ContainertextExample

Done

Levi Thomason mailto:notifications@github.com
24 June 2016 at 16:58

In docs/app/Examples/elements/Container/Types/ContainerTextExample.js
https://github.com/TechnologyAdvice/stardust/pull/277#discussion_r68419885:

@@ -0,0 +1,21 @@
+/* eslint-disable max-len */
+
+import React, { Component } from 'react'
+import { Container, Message } from 'stardust'
+
+export default class ContainerContainerExample extends Component {
-class ContainerContainerExample
+class ContainertextExample

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/TechnologyAdvice/stardust/pull/277/files/06fde8b6601dcbdb89260c966a8eba1a11f55df0#r68419885,
or mute the thread
https://github.com/notifications/unsubscribe/AAWHs-APioSkl5c_w7RXiAMgUdImf-P9ks5qO_6mgaJpZM4I9hiP.

Sent from Postbox
https://www.postbox-inc.com/?utm_source=email&utm_medium=siglink&utm_campaign=reach

levithomason · 2016-06-24T16:05:55Z

docs/app/Examples/elements/Container/Types/ContainerTextExample.js

+      <div>
+        <Message className='info'>
+          <p>A text container is a simpler markup alternative to using a grid with a single column, and is designed to have a reasonable maximum width for displaying flowing text</p>
+        </Message>


Since example file text appears on the site below the example, we want to keep it as minimal as possible. This <Message /> should be moved up to the ContainerTypesExamples.js by adding a closing tag to the < ComponentExample /> for this example file:

// ContainerTypesExamples.js ~ ln 14 <ComponentExample title='Text Container' description='A container can reduce its maximum width to more naturally accomodate a single column of text' examplePath='elements/Container/Types/ContainerTextExample' > <Message> // put the message here // it will be displayed above this example, but not in the example code </Message> </ComponentExample>

<h2> element replaced with Stardust Header.H2

levithomason · 2016-06-24T16:23:06Z

src/elements/Container/Container.js

+    useKeyOnly(left, 'left aligned'),
+    useKeyOnly(center, 'center aligned'),
+    useKeyOnly(right, 'right aligned'),
+    useKeyOnly(justified, 'justified'),


There are 4 types of possible class to props mappings for SUI classes. The alignment class here follows this API pattern:

aligned="left" // useValueAndKey(aligned, 'aligned') aligned="right" // useValueAndKey(aligned, 'aligned') aligned="center" // useValueAndKey(aligned, 'aligned')

One reason we use this key/value relationship is because two simultaneous alignment props are not valid:

<Container right left />

Where as making those values of the aligned prop means you can only have one at a time:

<Container aligned="left" />

However, the justified option breaks down however since the resulting class is not justified aligned. We've not yet run into this matchup yet so I'm open to suggestions. The best immediate thought I have is to still allow aligned="justified" for the API consistency, then in cx buildup:

cx( aligned === 'justified' ? 'justified' : useValueAndKey(aligned, 'aligned'), )

Thoughts?

EDIT

Fixed incorrect className suggestion.

I think your suggestion of just using a ternary operator is probably the best option. You could go down the route of adding unnecessary additions to propUtils but seems it's succinct enough to be used in this one isolated case.

we'd only require one useValueAndKey mapping rather than 3?

You are correct, I edited my typo shortly after submitting. See above.

For example, Button actions. It feels right that you'd declare a but this breaks down when stipulating that it should be left or right , but would seem even worse when combining it with an Icon:

<Button left action right icon/>

For a prop that may be boolean only or enum, there is the keyOrValueAndKey() util which allows us to do this (I assume you meant Input instead of Button):

<Input action /> // <input class="action" /> <Input action='left' /> // <input class="left action" /> <Input action='right' /> // <input class="right action" />

Or indeed, imposing defined props types with oneOf enums:

<Button align="left|center|right|justify" />

I think this is the way to go. You can see this in use in the Label for the pointing prop. A Label can be pointing alone or pointing with a direction, but never more than one.

I think with this one special handling for the justified case we have a pretty good solution:

cx( aligned === 'justified' ? 'justified' : useValueAndKey(aligned, 'aligned'), )

If the aligned prop is justified, then the class will be justified only. Otherwise, the class will be the value of the prop and it's key:

<Container aligned='justified' /> // <div class="ui container justified"></div> <Container aligned='left' /> // <div class="ui container left aligned"></div> <Container aligned='right' /> // <div class="ui container right aligned"></div>

Note, we won't be able to use the common test for propValueAndKeyToClassName since it has special handling for one of the values.

Since the alignment prop is pretty common, it may warrant it's own common.implementsAlignmentProp() test to handle the special justified case.

Yeah I'd digested more of your code after making that statement so
please disregard.

Have amended the Container alignment as per your suggestion and updated test

Levi Thomason wrote:

In src/elements/Container/Container.js
https://github.com/TechnologyAdvice/stardust/pull/277#discussion_r68436320:

{this.props.children}
-

)

}
+function Container(props) {

const {

text, left, center, right, justified, fluid,

children, className,

} = props
+

const classes = cx('sd-container ui',

useKeyOnly(text,'text'),

useKeyOnly(left,'left aligned'),

useKeyOnly(center,'center aligned'),

useKeyOnly(right,'right aligned'),

useKeyOnly(justified,'justified'),

we'd only require one useValueAndKey mapping rather than 3?

You are correct, I edited my typo shortly after submitting. See above.

For example, Button actions. It feels right that you'd declare a
but this breaks down when stipulating that it should be left or
right , but would seem even worse when combining it with an Icon:

||

For a prop that may be boolean only /or/ enum, there is the
|keyOrValueAndKey()| util which allows us to do this (I assume you
meant Input instead of Button):

//
//
//

Or indeed, imposing defined props types with oneOf enums:

|
|

I think this is the way to go. You can see this in use in the Label
for the pointing
https://github.com/TechnologyAdvice/stardust/blob/master/src/elements/Label/Label.js#L142

prop. A Label can be pointing alone or pointing with a direction, but
never more than one.

I think with this one special handling for the justified case we have
a pretty good solution:

cx(
aligned=== 'justified' ? 'justified' : useValueAndKey(aligned,'aligned'),
)

If the |aligned| prop is |justified|, then the class will be
|justified| only. Otherwise, the class will be the value of the prop
and it's key:

//

//

//

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/TechnologyAdvice/stardust/pull/277/files/f420da9c4eb10853132c436ba6a06faa82ffeba2#r68436320,

or mute the thread
https://github.com/notifications/unsubscribe/AAWHs9U_F5YTx6bTtZ-CBJqbKR0bn5vJks5qPBp5gaJpZM4I9hiP.

levithomason · 2016-06-24T16:26:36Z

test/specs/elements/Container/Container-test.js

+  it('adds the Container class', () => {
+    shallow(<Container />)
+      .should.have.className('container')
+  })


This test is automated as part of the common.isConformant() test. All components are required to include their Semantic UI component className.

So the common.propKeyOnlyToClassName are still required, but not the 2
'it' blocks?

Levi Thomason mailto:notifications@github.com
24 June 2016 at 17:26

In test/specs/elements/Container/Container-test.js
https://github.com/TechnologyAdvice/stardust/pull/277#discussion_r68423748:

common.propKeyOnlyToClassName(Container, 'text')

common.propKeyOnlyToClassName(Container, 'left', { className:
'aligned' })

common.propKeyOnlyToClassName(Container, 'center', { className:
'aligned' })

common.propKeyOnlyToClassName(Container, 'right', { className:
'aligned' })

common.propKeyOnlyToClassName(Container, 'justified')

common.propKeyOnlyToClassName(Container, 'fluid')
+

it('renders a
element', () => {

shallow()

.should.have.tagName('div')

})
+

it('adds the Container class', () => {

shallow()

.should.have.className('container')

})

This test is automated as part of the |common.isConformant()| test.
All components are required to include their Semantic UI component
className.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/TechnologyAdvice/stardust/pull/277/files/a083fd536fd2f416c6214e2af929e6c51b4e8d58#r68423748,
or mute the thread
https://github.com/notifications/unsubscribe/AAWHs8T9VMTH_JMod5KlJmmO1HaBUkeJks5qPAU9gaJpZM4I9hiP.

Sent from Postbox
https://www.postbox-inc.com/?utm_source=email&utm_medium=siglink&utm_campaign=reach

Sorry, should've been more explicit. You can keep all tests in this file, just remove this one:

it('adds the Container class', ...

All components must include a class that matches the component name. A <Container /> must at least include <div class="container" />, for example. Because this is true of all components, it is part of the base conformance test.

The conformance tests use the component's _meta object to ensure the className includes the component name.

levithomason · 2016-06-25T20:20:30Z

test/specs/elements/Container/Container-test.js

+  common.hasUIClassName(Container)
+
+  common.propKeyOnlyToClassName(Container, 'text')
+  common.propKeyAndValueToClassName(Container, 'aligned')


This is the test that is failing. This test will ensure that the component has the classNames:

left aligned
right aligned
center aligned
justified aligned <-- it is asserting this but it is wrong because we are overriding this value

I'm merging a new propUtil to handle the className logic. I'm also merging a new common test to ensure the aligned prop is correctly implemented. I think this makes sense since it has it's own className logic and it is used in many components.

Once merged, we'll be able to do this in cx:

cx( useAlignedProp(aligned), )

And this in tests:

common.implementsAlignedProp(Container)

I have now merged #285 which adds useAlignedProp propUtil and implementsAlignedProp common test.

The util and test account for the special handling of the justified value. You should be able to update this PR now.

implemented

…container

jhchill666 added 2 commits June 24, 2016 08:17

Container Component updated to v1 API

27dc01c

- Container Component updated to v1 API - Examples updated to match Semantic Docs - Container-test added to suite

Fixes lint errors

a5441e5

jhchill666 added 5 commits June 24, 2016 08:48

Merge branch 'master' into feature/container

7631a91

eslint-disable max-len for long example paragraphs

d64f2b2

eslint-disable max-len for long example paragraphs

84f9f53

Merge remote-tracking branch 'origin/feature/container' into feature/…

9e2d9b4

…container

fixes lint errors

06fde8b

levithomason reviewed Jun 24, 2016
View reviewed changes

jhchill666 added 2 commits June 24, 2016 17:02

Function name amended to match Class name

724759a

Function name amended to match Class name

bd4b8de

levithomason reviewed Jun 24, 2016
View reviewed changes

jhchill666 added 3 commits June 24, 2016 17:09

Message relocated from example component to examples container

241a022

<h2> element replaced with Stardust Header.H2

f420da9

Message relocated from example component to examples container

a083fd5

<h2> element replaced with Stardust Header.H2

levithomason reviewed Jun 24, 2016
View reviewed changes

Redundant ContainerComponent element removed in favour of tag inline

bd628e3

levithomason reviewed Jun 24, 2016
View reviewed changes

jhchill666 added 3 commits June 24, 2016 18:36

Removes redundant Container class check - handled in base tests

77b4bb5

Alignment constrained to alignment prop

0d50e12

[Test] Alignment constrained to alignment prop

edf673a

levithomason mentioned this pull request Jun 24, 2016

Update legacy component APIs to the v1 API #269

Closed

14 tasks

jhchill666 added 2 commits June 25, 2016 10:46

fixes lint errors

9bb4509

Merge branch 'master' into feature/container

f31fe72

levithomason reviewed Jun 25, 2016
View reviewed changes

jhchill666 added 6 commits June 26, 2016 06:59

Image Component updated to v1 API

ad81023

Image Component updated to v1 API

6330ca0

Merge branch 'master' into feature/container

f2781cd

Implements useAlignedProp / common.implementAlignProp

2366b67

Merge remote-tracking branch 'origin/feature/container' into feature/…

8863f6d

…container

Fixes lint errors

28f6d7f

levithomason merged commit 54723c3 into Semantic-Org:master Jun 26, 2016

jhchill666 deleted the feature/container branch June 27, 2016 05:09

levithomason added the enhancement label Jul 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Component updated to v1 API #277

Container Component updated to v1 API #277

jhchill666 commented Jun 24, 2016

codecov-io commented Jun 24, 2016 •

edited

Loading

levithomason Jun 24, 2016

jhchill666 Jun 24, 2016

levithomason Jun 24, 2016

jhchill666 Jun 26, 2016

levithomason Jun 24, 2016 •

edited

Loading

jhchill666 Jun 24, 2016

levithomason Jun 24, 2016

levithomason Jun 24, 2016

jhchill666 Jun 24, 2016

levithomason Jun 24, 2016

jhchill666 Jun 24, 2016

levithomason Jun 24, 2016

jhchill666 Jun 26, 2016

levithomason Jun 25, 2016

levithomason Jun 25, 2016

jhchill666 Jun 26, 2016

Container Component updated to v1 API #277

Container Component updated to v1 API #277

Conversation

jhchill666 commented Jun 24, 2016

codecov-io commented Jun 24, 2016 • edited Loading

Current coverage is 86.00%

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 Jun 24, 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

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

codecov-io commented Jun 24, 2016 •

edited

Loading

levithomason Jun 24, 2016 •

edited

Loading