feat(datalist): add hidden and visible breakpoints by boaz0 · Pull Request #2251 · patternfly/patternfly-react · GitHub

What:

closes #2221 and fixes #2197

update the datalist action component to what is given in pf-core
add the hidden and visible breakpoint modifiers as props
update the datalist checkbox and actions example in the docs
update tests

Codecov Report

❗ No coverage uploaded for pull request base (master@7e9ae5c). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2251   +/-   ##
=========================================
  Coverage          ?   80.59%           
=========================================
  Files             ?      666           
  Lines             ?     8441           
  Branches          ?      719           
=========================================
  Hits              ?     6803           
  Misses            ?     1280           
  Partials          ?      358

Flag	Coverage Δ
#patternfly3	85.23% <ø> (?)
#patternfly4	76.16% <100%> (?)
#patternflymisc	95.79% <ø> (?)

Impacted Files	Coverage Δ
...act-core/src/components/DataList/DataListAction.js	77.77% <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 7e9ae5c...71e2477. Read the comment docs.

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

Thanks @boaz0! Does this only allow you to pass a single breakpoint to visibleBreakpoint or hiddenBreakpoint? If so, we'll want to extend it so you can, for example, show at xs (0), hide on sm, show on md, hide on lg, and show again on xl.

And it looks like hiddenBreakpoint="" will hide something at all breakpoints? Personally I think that's fine as long as it's documented, but since you're passing breakpoints, seems like you should also be able to pass "xs" (0) there to do the same thing.

And since this is the same exact functionality that we use in the table component with this syntax classNames(Visibility.hidden, Visibility.visibleOnMd, Visibility.hiddenOnLg), and we'll likely add this functionality to other components in the future, I wonder if we should use a similar approach in the classes or names used to show/hide content?

@mcoker thanks for your feedback ❤️ ! I will update this PR to match the requirements. 😄

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

@mcoker & @redallen - PR is updated. Feel free to review 😄 thanks.

Left one comment, otherwise looks really good

is isOpen3 missing here?

That's odd I thought I added it. Thanks for the double check.

LGTM

lgtm

Just want to confirm this should be [hidden/visible]On2Xl and not [hidden/visible]On2xl? Not sure how numbers play with camel case.

Hmm... I'm trying the [hidden/visible]On2Xl props and they don't seem to be working. Can someone confirm? @tlabaj or @christiemolloy. I'm probably doing it wrong :)

I actually was wondering the same thing...

hiddenOn2Xl is correct. It comes from our Javascript variable names being camelcased in react-styles based on the css variables.

I also do not see the [hidden/visible]On2Xl being applied.

For now I wrote a workaround for that. Please review and let me know what you think.

Thanks.

Investigating Michael's comments

Workaround looks good. Thanks as always @boaz0 ! 👍

@boaz0 the preview URL isn't working for some reason, but I built it locally and looks like the 2xl breakpoints are working, but looks like the base DataListActionVisibility.hidden isn't working.

The buttons are still visible when i reduce the screen width

Thanks. I will be looking into it. 😄

Yep, I found the problem!

I am just adding unit tests.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

Naming comments were investigated and fixed.

Your changes have been released in:

@patternfly/react-charts@4.4.13
@patternfly/react-core@3.65.0
@patternfly/react-docs@4.8.63
@patternfly/react-inline-edit-extension@2.9.26
demo-app-ts@2.6.1
@patternfly/react-styled-system@3.6.10
@patternfly/react-styles@3.5.0
@patternfly/react-table@2.14.0
@patternfly/react-topology@2.5.7
@patternfly/react-virtualized-extension@1.1.59

Thanks for your contribution! 🎉

Thanks a lot: @jschuler @redallen @mcoker @tlabaj and @christiemolloy

@boaz0 @mcoker @redallen
This PR was merged before it was ready. the 2Xl breakpoints are still not working, I have opened issue #2490 for follow up work.

@tlabaj can you give me a way to reproduce it in https://patternfly-react.surge.sh/patternfly-4/components/datalist/ or any way that I can see where it's not working.

I tried it on https://patternfly-react.surge.sh/patternfly-4/ and it looks OK.

@boaz0 @tlabaj I also just confirmed it looks OK on master. I was testing previously using the preview build in this PR. Maybe the preview wasn't updating properly. I think we can close #2490

tlabaj requested a review from mcoker June 17, 2019 18:28

tlabaj added PF4 and removed css approved labels Jun 17, 2019

redallen approved these changes Jun 20, 2019

View reviewed changes

redallen suggested changes Jun 20, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/components/DataList/DataListAction.js Outdated

tlabaj assigned redallen Jun 25, 2019

redallen previously approved these changes Jun 26, 2019

View reviewed changes

tlabaj suggested changes Jun 27, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/components/DataList/DataList.md

christiemolloy suggested changes Jun 27, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-core/src/components/DataList/DataList.md Outdated

boaz0 dismissed redallen’s stale review via 018e3a8 June 28, 2019 16:41

tlabaj suggested changes Jul 1, 2019

View reviewed changes

tlabaj approved these changes Jul 2, 2019

View reviewed changes

christiemolloy previously approved these changes Jul 2, 2019

View reviewed changes

mcoker reviewed Jul 2, 2019

View reviewed changes

tlabaj previously requested changes Jul 2, 2019

View reviewed changes

boaz0 dismissed christiemolloy’s stale review via 8b10747 July 5, 2019 03:53

redallen previously approved these changes Jul 5, 2019

View reviewed changes

jschuler suggested changes Jul 5, 2019

View reviewed changes

boaz0 dismissed redallen’s stale review via d82580c July 5, 2019 19:00

feat(datalist): add hidden and visible breakpoints

71e2477

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

jschuler approved these changes Jul 8, 2019

View reviewed changes

redallen approved these changes Jul 8, 2019

View reviewed changes

redallen merged commit fdbde6d into patternfly:master Jul 8, 2019

boaz0 deleted the closes_2221 branch July 8, 2019 23:51

tlabaj mentioned this pull request Jul 9, 2019

PF4 Datalist: hiddenOn2Xl and visibleOn2Xl breakpoints do not work #2490

Closed

                 constructor(props) {
                   super(props);
-                  this.state = { isOpen: false };
+                  this.state = { isOpen1: false, isOpen2: false };

+                'hiddenOnMd',
+                'hiddenOnLg',
+                'hiddenOnXl',
+                'hiddenOn2Xl',

+                'hiddenOnMd',
+                'hiddenOnLg',
+                'hiddenOnXl',
+                'hiddenOn_2xl',

+                            />
+                          </DataListAction>
+                          <DataListAction
+                            className={css(DataListActionVisibility.visibleOnXl, DataListActionVisibility.hidden)}

feat(datalist): add hidden and visible breakpoints by boaz0 · Pull Request #2251 · patternfly/patternfly-react · GitHub

Uh oh!

feat(datalist): add hidden and visible breakpoints - #2251

feat(datalist): add hidden and visible breakpoints#2251redallen merged 1 commit intopatternfly:masterpatternfly/patternfly-react:masterfrom boaz0:closes_2221Copy head branch name to clipboard

Conversation

boaz0 commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

patternfly-build commented Jun 17, 2019

Uh oh!

mcoker commented Jun 18, 2019

Uh oh!

Uh oh!

boaz0 commented Jun 21, 2019

Uh oh!

patternfly-build commented Jun 21, 2019

Uh oh!

boaz0 commented Jun 25, 2019

Uh oh!

Uh oh!

christiemolloy left a comment

Choose a reason for hiding this comment

Uh oh!

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!

christiemolloy 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!

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!

mcoker commented Jul 5, 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!

patternfly-build commented Jul 8, 2019

Uh oh!

boaz0 commented Jul 8, 2019

Uh oh!

tlabaj commented Jul 9, 2019

Uh oh!

boaz0 commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

feat(datalist): add hidden and visible breakpoints#2251
redallen merged 1 commit into
patternfly:masterfrom
boaz0:closes_2221

boaz0 commented Jun 12, 2019 •
edited
Loading

codecov-io commented Jun 15, 2019 •
edited
Loading

boaz0 commented Jul 9, 2019 •
edited
Loading