feat(divider): add fitted prop #333

gopalgoel19 · 2018-10-08T12:26:41Z

Divider `fitted` prop

A divider can be fitted to render a divider that is simply a line with no space above or below it.

TODO

API Proposal

`fitted`

Use cases:

A fitted divider can be used inside a List component to separate few list items from another.

<List>
    <List.Item
      media={<Image src="public/images/avatar/small/matt.jpg" avatar />}
      header="Irving Kuhic"
      content="Program the sensor to the SAS alarm through the haptic SQL card!"
    />
    <List.Item
      media={<Image src="public/images/avatar/small/steve.jpg" avatar />}
      header="Skyler Parks"
      content="Use the online FTP application to input the multi-byte application!"
    />
    <Divider fitted size={1}/>
    <List.Item
      media={<Image src="public/images/avatar/small/nom.jpg" avatar />}
      header="Dante Schneider"
      content="The GB pixel is down, navigate the virtual interface!"
    />
  </List>

The above code give us something like,

codecov · 2018-10-08T12:26:45Z

Codecov Report

Merging #333 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   89.73%   89.73%           
=======================================
  Files          64       64           
  Lines        1237     1237           
  Branches      180      180           
=======================================
  Hits         1110     1110           
  Misses        125      125           
  Partials        2        2

Impacted Files	Coverage Δ
src/components/Divider/Divider.tsx	`100% <ø> (ø)`	⬆️

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 6369b85...bad5f5e. Read the comment docs.

codepretty · 2018-10-08T17:50:25Z

src/themes/teams/components/Divider/dividerStyles.ts

    return {
      color: variables.textColor,
      display: 'flex',
      alignItems: 'center',
-      paddingTop: pxToRem(variables.dividerPadding),


The default padding should already be 0, so I think you only need to apply padding for the non-fitted case.

...(!fitted && { paddingTop: pxToRem(variables.dividerPadding), paddingBottom: pxToRem(variables.dividerPadding), }),

hughreeling · 2018-10-09T11:21:33Z

You should NOT do this:

 <List>
     <List.Item
       media={<Image src="public/images/avatar/small/matt.jpg" avatar />}
       header="Irving Kuhic"
       content="Program the sensor to the SAS alarm through the haptic SQL card!"
     />
     <List.Item
       media={<Image src="public/images/avatar/small/steve.jpg" avatar />}
header="Skyler Parks"
       content="Use the online FTP application to input the multi-byte application!"
     />
     **<Divider fitted size={1}/>**
     <List.Item
       media={<Image src="public/images/avatar/small/nom.jpg" avatar />}
       header="Dante Schneider"
       content="The GB pixel is down, navigate the virtual interface!"
     />
   </List>

A list should not contain any direct children apart from list items. This will definitely break things.

gopalgoel19 · 2018-10-09T12:21:03Z

@hughreeling What would be the best way to have a similar line in between list items?

Considering what you mentioned above, my present options are:

Add a border to the bottom of the list item when required.
Insert a Divider inside the ListItem.
Make a ListItem serve as a divider itself. E.g. <List.Item divider/> renders a list item which look and feel like the Divider.
Use two separate List component.

Please let me know if there are other options as well

hughreeling · 2018-10-09T14:02:48Z

Lists definitely need to contain only list items. ( https://dev.w3.org/html5/html-author/#the-ol-element) I think the options are for the list item to contain the divider, or to style the list item. Currently you could so something like <Divider as='li' /> but I highly recommend NOT doing this, it's pretty much certain to make accessibility / keyboard navigation.

…

On Tue, Oct 9, 2018 at 3:26 PM Gopal Goel ***@***.***> wrote: @hughreeling <https://github.com/hughreeling> What would be the best way to have a similar line in between list items? Considering what you mentioned above, my present options are: 1. Add a border to the bottom of the list item when required. 2. Insert a Divider inside the ListItem. 3. Make a ListItem serve as a divider itself. E.g. <List.Item divider/> renders a list item which look and feel exactly like the Divider. Please let me know if there are other options as well — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#333 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGjGSzs8tA4ebgnxf2RX9TrTUiQ4Q1FXks5ujJSwgaJpZM4XM02o> .

hughreeling · 2018-10-09T14:04:17Z

it's pretty much certain to break accessibility / keyboard navigation.

…

On Tue, Oct 9, 2018 at 4:02 PM Hugh Eland ***@***.***> wrote: Lists definitely need to contain only list items. ( https://dev.w3.org/html5/html-author/#the-ol-element) I think the options are for the list item to contain the divider, or to style the list item. Currently you could so something like <Divider as='li' /> but I highly recommend NOT doing this, it's pretty much certain to make accessibility / keyboard navigation. On Tue, Oct 9, 2018 at 3:26 PM Gopal Goel ***@***.***> wrote: > @hughreeling <https://github.com/hughreeling> What would be the best way > to have a similar line in between list items? > > Considering what you mentioned above, my present options are: > > 1. Add a border to the bottom of the list item when required. > 2. Insert a Divider inside the ListItem. > 3. Make a ListItem serve as a divider itself. E.g. <List.Item > divider/> renders a list item which look and feel exactly like the > Divider. > > Please let me know if there are other options as well > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#333 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGjGSzs8tA4ebgnxf2RX9TrTUiQ4Q1FXks5ujJSwgaJpZM4XM02o> > . >

kuzhelov · 2018-10-09T15:46:44Z

docs/src/examples/components/Divider/Variations/DividerExampleFitted.tsx

+import React from 'react'
+import { Divider } from '@stardust-ui/react'
+
+const DividerExampleFitted = () => (


this example could be eliminated - as there is no Children API variation for this case

kuzhelov · 2018-10-09T15:47:58Z

src/components/Divider/Divider.tsx

@@ -43,6 +43,9 @@ class Divider extends UIComponent<Extendable<IDividerProps>, any> {
    /** Shorthand for primary content. */
    content: customPropTypes.contentShorthand,

+    /** A divider can be fitted, without any space above or below it.  */
+    divider: PropTypes.bool,


this should be name as fitted. Also, please, make sure that IDividerProps reflects the changes you've introduced for the API.

In general, would suggest to make tests in docs (probably, you've done that), but with dev console being opened - in that case you won't let these problems pass your checks :)

Ah, a silly one. Fixing it right away.

kuzhelov

please, address the changes raised in comments before merging

gopalgoel19 requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners October 8, 2018 12:26

codepretty reviewed Oct 8, 2018

View reviewed changes

gopalgoel19 force-pushed the feat/divider-fitted-prop branch from ac99743 to 40b6e1a Compare October 9, 2018 04:53

feat(divider): add fitted prop

30e4ec6

gopalgoel19 force-pushed the feat/divider-fitted-prop branch from 40b6e1a to 30e4ec6 Compare October 9, 2018 05:24

gopalgoel19 added the 🚀 ready for review label Oct 9, 2018

kuzhelov reviewed Oct 9, 2018

View reviewed changes

kuzhelov suggested changes Oct 9, 2018

View reviewed changes

kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Oct 9, 2018

Fix typos

b4abb10

kuzhelov approved these changes Oct 10, 2018

View reviewed changes

kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Oct 10, 2018

kuzhelov-ms added 2 commits October 10, 2018 15:17

Merge branch 'master' into feat/divider-fitted-prop

84a0568

update changelog

bad5f5e

kuzhelov merged commit 47f8289 into master Oct 10, 2018

kuzhelov deleted the feat/divider-fitted-prop branch November 19, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(divider): add fitted prop #333

feat(divider): add fitted prop #333

gopalgoel19 commented Oct 8, 2018 •

edited by kuzhelov

Loading

codecov bot commented Oct 8, 2018 •

edited

Loading

codepretty Oct 8, 2018

gopalgoel19 Oct 9, 2018

hughreeling commented Oct 9, 2018 •

edited

Loading

gopalgoel19 commented Oct 9, 2018 •

edited

Loading

hughreeling commented Oct 9, 2018 via email

hughreeling commented Oct 9, 2018 via email

kuzhelov Oct 9, 2018

kuzhelov Oct 9, 2018 •

edited

Loading

gopalgoel19 Oct 10, 2018 •

edited

Loading

kuzhelov left a comment

feat(divider): add fitted prop #333

feat(divider): add fitted prop #333

Conversation

gopalgoel19 commented Oct 8, 2018 • edited by kuzhelov Loading

Divider fitted prop

TODO

API Proposal

fitted

Use cases:

codecov bot commented Oct 8, 2018 • edited Loading

Codecov Report

codepretty Oct 8, 2018

Choose a reason for hiding this comment

gopalgoel19 Oct 9, 2018

Choose a reason for hiding this comment

hughreeling commented Oct 9, 2018 • edited Loading

gopalgoel19 commented Oct 9, 2018 • edited Loading

hughreeling commented Oct 9, 2018 via email

hughreeling commented Oct 9, 2018 via email

kuzhelov Oct 9, 2018

Choose a reason for hiding this comment

kuzhelov Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

gopalgoel19 Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

kuzhelov left a comment

Choose a reason for hiding this comment

gopalgoel19 commented Oct 8, 2018 •

edited by kuzhelov

Loading

Divider `fitted` prop

`fitted`

codecov bot commented Oct 8, 2018 •

edited

Loading

hughreeling commented Oct 9, 2018 •

edited

Loading

gopalgoel19 commented Oct 9, 2018 •

edited

Loading

kuzhelov Oct 9, 2018 •

edited

Loading

gopalgoel19 Oct 10, 2018 •

edited

Loading