feat(Divider): add divider component by christiemolloy · Pull Request #3125 · patternfly/patternfly-react · GitHub

PatternFly-React preview: https://patternfly-react-pr-3125.surge.sh

Can you call the prop component to be consistent with other components in the library

could you explain what you mean here :)

Our other components name the prop 'component' rather than variant when we want to change the html tag.

thanks updated

no need for a role prop. Looks like it should be set to separator for div or li . We should do that conditionally in in the component when the 'component prop is not an hr.

Can you spread props at the end here so they take precedence.

after you update the component you can update these to remove the role='separator'

Can you revert the changes in this file. It should not have been updated with the changes you made.

Codecov Report

Merging #3125 into master will increase coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #3125      +/-   ##
==========================================
+ Coverage   69.02%   69.04%   +0.01%     
==========================================
  Files         858      859       +1     
  Lines       23535    23549      +14     
  Branches     1877     1880       +3     
==========================================
+ Hits        16246    16259      +13     
  Misses       6336     6336              
- Partials      953      954       +1

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	69.23% <ø> (ø)	⬆️
#patternfly4	68.14% <92.85%> (+0.03%)	⬆️

Impacted Files	Coverage Δ
...re/src/experimental/components/Divider/Divider.tsx	92.85% <92.85%> (ø)

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 8dcfb49...a25e8d1. Read the comment docs.

you can remove role="seperator"

Our other components name the prop 'component' rather than variant when we want to change the html tag.

I think you want to do something like this instead.

Suggested change

      
                <Component className={css(styles.divider, className)} role={ variant!='hr' ? 'separator' : undefined } {...props}/>
          
                <Component className={css(styles.divider, className)} {...(variant != 'hr' && {role : 'seperator' })}  {...props}/>

That makes sense thank you @tlabaj !

Looking great Christie. A few more small comments.

Did you mean for this to be a function? e.g.

Suggested change

      
            DividerHr = (
          
            DividerHr = () => (

same comment for other examples.

Can you change the prop descriptions to Sentence case.

LGTM

Suggested change

      
                <Component className={css(styles.divider, className)} {...(component != 'hr' && {role : 'seperator' })} {...props} />
          
                <Component className={css(styles.divider, className)} {...(component != 'hr' && {role : 'separator' })} {...props} />

Suggested change

      
              role="seperator"
          
              role="separator"

Suggested change

      
              role="seperator"
          
              role="separator"

😀👍

LGTM

christiemolloy added 4 commits October 10, 2019 12:07

feat(divider): add divider component

2181ee7

feat(divider): add tests

59a0118

feat(divider): update tests

5cd7fe9

fix(divider): update tests

0ec1d8b

christiemolloy requested review from jessiehuff, kmcfaul, mcoker and tlabaj October 11, 2019 15:38

tlabaj added the css review label Oct 11, 2019

tlabaj suggested changes Oct 11, 2019

View reviewed changes

tlabaj self-assigned this Oct 11, 2019

fix(divider): updates

7dcf939

tlabaj suggested changes Oct 11, 2019

View reviewed changes

christiemolloy added 5 commits October 14, 2019 10:01

fix(divider): rebase yarn file

97fe302

fix(divider): updates based on review

5dd3936

fix(divider): push more updates

2c6c911

fix(divider): rebase yarn

d970850

fix(divider): push updates

eacad89

tlabaj suggested changes Oct 14, 2019

View reviewed changes

fix(divider): update based on titani comments

95bd573

tlabaj previously approved these changes Oct 16, 2019

View reviewed changes

mcoker requested changes Oct 21, 2019

View reviewed changes

fix(divider): update spelling

a25e8d1

christiemolloy dismissed tlabaj’s stale review via a25e8d1 October 21, 2019 21:12

mcoker approved these changes Oct 21, 2019

View reviewed changes

tlabaj approved these changes Oct 22, 2019

View reviewed changes

kmcfaul approved these changes Oct 22, 2019

View reviewed changes

tlabaj merged commit 92971d3 into patternfly:master Oct 22, 2019

+                /** Adds a role to the divider for accessibility */
+                'role'?: string;
+                /** the component type to use */
+                variant?: 'hr' | 'li' | 'div' ;

+                /** additional classes added to the Badge */
+                className?: string;
+                /** Adds a role to the divider for accessibility */
+                'role'?: string;

               "@patternfly/patternfly@2.33.8":
                 version "2.33.8"
                 resolved "https://registry.yarnpkg.com/@patternfly/patternfly/-/patternfly-2.33.8.tgz#2885847b4c49d8606abed17a0415da24fec222aa"
+                integrity sha512-tkT1GrdIyvGiDvNHATOa7XLMLKdztpra8ngS8qhCcEWBKh9MWNj5ysjOrkna32bSF9GEkR4TFX492W8ESTHWvQ==

+              DividerLi = (
+                <ul>
+                  <li>List item one</li>
+                  <Divider variant="li" role="separator"/>

+                /** Adds a role to the divider for accessibility */
+                'role'?: string;
+                /** the component type to use */
+                variant?: 'hr' | 'li' | 'div' ;

+                const Component: any = variant;
+                return (
+                  <Component {...props} className={css(styles.divider, className)} role={role}/>

+              import { Divider } from '@patternfly/react-core/dist/esm/experimental';
+              DividerLi = (
+                <Divider variant="div" role="separator"/>

	<Component className={css(styles.divider, className)} role={ variant!='hr' ? 'separator' : undefined } {...props}/>
	<Component className={css(styles.divider, className)} {...(variant != 'hr' && {role : 'seperator' })} {...props}/>

+              exports[`divider using div 1`] = `
+              <div
+                className="pf-c-divider"
+                role="seperator"

feat(Divider): add divider component by christiemolloy · Pull Request #3125 · patternfly/patternfly-react · GitHub

Uh oh!

feat(Divider): add divider component - #3125

feat(Divider): add divider component#3125tlabaj merged 12 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom christiemolloy:dividerchristiemolloy/patternfly-react:dividerCopy head branch name to clipboard

Conversation

christiemolloy commented Oct 11, 2019

Uh oh!

patternfly-build commented Oct 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

feat(Divider): add divider component#3125
tlabaj merged 12 commits into
patternfly:masterfrom
christiemolloy:divider

tlabaj Oct 11, 2019 •
edited
Loading

codecov-io commented Oct 11, 2019 •
edited
Loading