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

fix(datalist): adds breakpoints for actions #1886

Merged

Conversation

srambach
Copy link
Member

@srambach srambach commented Jun 4, 2019

This collapses actions into a kebab menu by using modifiers specifying what breakpoint to show/hide the menu toggle and action buttons.

Closes #1704

@patternfly-build
Copy link

patternfly-build commented Jun 4, 2019

Deploy preview for pf-next ready!

Built with commit 7fe5748

https://deploy-preview-1886--pf-next.netlify.com

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This looks good to me @srambach !

@@ -80,6 +80,7 @@
--pf-c-data-list__item-action--not-last-child--MarginRight: var(--pf-global--spacer--md);
--pf-c-data-list__item-action--not-last-child--lg--MarginBottom: var(--pf-global--spacer--md);
--pf-c-data-list__action--MarginTop: calc(var(--pf-global--spacer--form-element) * -1);
--pf-c-data-list__item-action--hidden-visible--Display: flex;
Copy link
Contributor

@mcoker mcoker Jun 4, 2019

Choose a reason for hiding this comment

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

how about we change this to --pf-c-data-list__item-action--Display: flex;, then use var(--pf-c-data-list__item-action--Display) for the @include on L182 and for the display property on L184 since those should always match?

@@ -178,6 +179,8 @@
}

.pf-c-data-list__item-action {
@include pf-hidden-visible(var(--pf-c-data-list__item-action--hidden-visible--Display));
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

🥇

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

At here, should these action buttons wrap by default?

Screen Shot 2019-06-05 at 1 50 51 PM

This is set on the actions element

  @media screen and (max-width: $pf-global--breakpoint--lg) {
    flex-wrap: wrap;
  }

I know that if we change the breakpoint to be larger, then even two action buttons will wrap. But 4 buttons like that seems like alot. We probably also want to avoid having a max-width on the action container im sure.. what if we had a modifier called pf-m-wrap that allows the actions element to wrap if the user wanted it to. Then we can remove that media query. it doesnt seem flexible enough.

@srambach
Copy link
Member Author

srambach commented Jun 5, 2019

I read the issue as not wanting a wrap in lieu of the breakpoints but @mcarrano can you clarify whether you want it to wrap or not?

@mcarrano
Copy link
Member

mcarrano commented Jun 5, 2019

When you say wrap here, are you asking if we should start to stack the buttons below a certain breakpoint? That was not my intent. I think that we want to collapse the buttons into the kabob when they no longer fit. Keep in mind that the example here is not a likely one or a design I would encourage. I would not recommend including more than one or two inline action buttons in a row. Maybe we should remove that example?

@srambach
Copy link
Member Author

srambach commented Jun 5, 2019

I can make the example whatever you want. I was showing that if you did have a lot, it could collapse earlier than if you had fewer. I assume you would never collapse 1 button, so do you just want one example with 2 buttons that collapse into a kebab? @mcarrano

@mcarrano
Copy link
Member

mcarrano commented Jun 5, 2019

I don't feel strongly about whether the 4 button example should be removed or not. I'm struggling with whether you would collapse one button or not. What if you scale down to mobile? A single button could occupy half the screen. But then it's odd to have a kabob with one action. @srambach @christiemolloy what would you expect?

@srambach
Copy link
Member Author

srambach commented Jun 5, 2019

I would not want a single button to collapse to a kebab. But it would just not be coded with the modifier saying to collapse it. Or maybe that single button would switch to an icon button. That would be possible with the changes in this PR. This PR gives you the mechanism to show/hide actions depending on a breakpoint. @mcarrano

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

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

Sounds good to me @mcarrano @srambach

@mcoker mcoker merged commit ddcf3f5 into patternfly:master Jun 6, 2019
@patternfly-build
Copy link

🎉 This PR is included in version 2.12.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants