Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(divider): add fitted prop #333

Merged
merged 4 commits into from
Oct 10, 2018
Merged

feat(divider): add fitted prop #333

merged 4 commits into from
Oct 10, 2018

Conversation

gopalgoel19
Copy link
Member

@gopalgoel19 gopalgoel19 commented Oct 8, 2018

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

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

API Proposal

fitted

screenshot 52

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,
screenshot 53

@codecov
Copy link

codecov bot commented Oct 8, 2018

Codecov Report

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

Impacted file tree graph

@@           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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
}),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@hughreeling
Copy link
Contributor

hughreeling commented Oct 9, 2018

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
Copy link
Member Author

gopalgoel19 commented Oct 9, 2018

@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 like the Divider.
  4. Use two separate List component.

Please let me know if there are other options as well

@hughreeling
Copy link
Contributor

hughreeling commented Oct 9, 2018 via email

@hughreeling
Copy link
Contributor

hughreeling commented Oct 9, 2018 via email

import React from 'react'
import { Divider } from '@stardust-ui/react'

const DividerExampleFitted = () => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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,
Copy link
Contributor

@kuzhelov kuzhelov Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member Author

@gopalgoel19 gopalgoel19 Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, a silly one. Fixing it right away.

Copy link
Contributor

@kuzhelov kuzhelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, address the changes raised in comments before merging

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Oct 9, 2018
@kuzhelov kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Oct 10, 2018
@kuzhelov kuzhelov merged commit 47f8289 into master Oct 10, 2018
@kuzhelov kuzhelov deleted the feat/divider-fitted-prop branch November 19, 2018 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants