Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 8, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Jun 12, 2019

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
@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch 3 times, most recently from 9895536 to 92eae03 Jun 15, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 15, 2019

Codecov Report

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

Impacted file tree graph

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

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch from 92eae03 to 660e64a Jun 17, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 17, 2019

@tlabaj tlabaj requested a review from mcoker Jun 17, 2019
@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Jun 18, 2019

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?

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 21, 2019

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

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch 2 times, most recently from c50ff9d to 68fe9a6 Jun 21, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 21, 2019

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

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch 2 times, most recently from 4401a7a to f4d2460 Jun 21, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jun 25, 2019

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

Copy link
Member

christiemolloy left a comment

Left one comment, otherwise looks really good

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch 3 times, most recently from 6b9a6b9 to c142fd3 Jun 28, 2019
@@ -96,15 +98,35 @@ import {
class CheckboxActionDataList extends React.Component {
constructor(props) {
super(props);
this.state = { isOpen: false };
this.state = { isOpen1: false, isOpen2: false };

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 1, 2019

Contributor

is isOpen3 missing here?

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 1, 2019

Author Member

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

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch from c142fd3 to 0f537b7 Jul 1, 2019
@tlabaj
tlabaj approved these changes Jul 2, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Member

christiemolloy left a comment

lgtm

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

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@mcoker

mcoker Jul 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 2, 2019

Contributor

I actually was wondering the same thing...

This comment has been minimized.

Copy link
@redallen

redallen Jul 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@tlabaj

tlabaj Jul 2, 2019

Contributor

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

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 5, 2019

Author Member

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

Thanks.

Copy link
Contributor

tlabaj left a comment

Investigating Michael's comments

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch from 0f537b7 to 8b10747 Jul 5, 2019
'hiddenOnMd',
'hiddenOnLg',
'hiddenOnXl',
'hiddenOn_2xl',

This comment has been minimized.

Copy link
@redallen

redallen Jul 5, 2019

Contributor

Workaround looks good. Thanks as always @boaz0 ! 👍

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Jul 5, 2019

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

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

This comment has been minimized.

Copy link
@jschuler

jschuler Jul 5, 2019

Collaborator

The buttons are still visible when i reduce the screen width

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 5, 2019

Author Member

Thanks. I will be looking into it. 😄

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 5, 2019

Author Member

Yep, I found the problem!

This comment has been minimized.

Copy link
@boaz0

boaz0 Jul 5, 2019

Author Member

I am just adding unit tests.

@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch from 8b10747 to d82580c Jul 5, 2019
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2221 branch from d82580c to 71e2477 Jul 5, 2019
@redallen redallen dismissed tlabaj’s stale review Jul 8, 2019

Naming comments were investigated and fixed.

@redallen redallen merged commit fdbde6d into patternfly:master Jul 8, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 8, 2019

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

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 8, 2019

@boaz0 boaz0 deleted the boaz0:closes_2221 branch Jul 8, 2019
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jul 9, 2019

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

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Jul 9, 2019

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

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Jul 9, 2019

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.