feat(table): add support to hide/show cells at breakpoints #1748
Conversation
|
Deploy preview for pf-next ready! Built with commit be24186a040f93a8aea74efbc5ffdbbcbf52dcf1 |
|
Deploy preview for pf-next ready! Built with commit 69c8ce5 |
mcoker
force-pushed
the
issue-1725
branch
2 times, most recently
from
e2665e9 to
7e8ceef
Compare
There was a problem hiding this comment.
Looks great! The only change I see is to add :not(.pf-m-hidden) to: https://github.com/patternfly/patternfly-next/blob/7e8ceef806982472c185ee666c6691465cb10502/src/patternfly/components/Table/table-grid.scss#L199
| &.pf-m-center { | ||
| text-align: center; | ||
| } | ||
|
|
||
| // Hide/show cells at different breakpoints | ||
| @include pf-hidden-visible("table-cell"); | ||
| } |
| ### Usage | ||
| | Class | Applied To | Outcome | | ||
| | -- | -- | -- | | ||
| | `.pf-m-hidden{-on-[breakpoint]}` | `.pf-c-table tr > *` | Hides a table cell at a given breakpoint, or hides it at all breakpoints with `.pf-m-hidden`. | |
There was a problem hiding this comment.
Maybe make a point that you need to apply the same breakpoint to all cells within the same column, thats something i didnt even think about when i first saw this PR
| @@ -152,3 +152,70 @@ | |||
| scrollbar-width: none; // hides scrollbars in Firefox 64 and up | |||
| -ms-overflow-style: -ms-autohiding-scrollbar; // auto hides scrollbars in Edge | |||
| } | |||
|
|
|||
| @mixin pf-hidden-visible($val: "block") { | |||
There was a problem hiding this comment.
Were you thinking of updating the other components that use display: none; visibility: hidden within this PR?
There was a problem hiding this comment.
oh nah, I wasn't planning on it. this mixin was just for the hidden/visible functionality.
| @@ -152,3 +152,70 @@ | |||
| scrollbar-width: none; // hides scrollbars in Firefox 64 and up | |||
| -ms-overflow-style: -ms-autohiding-scrollbar; // auto hides scrollbars in Edge | |||
| } | |||
|
|
|||
| @mixin pf-hidden-visible($val: "block") { | |||
| &.pf-m-hidden { | |||
There was a problem hiding this comment.
I feel like we should make a note that you dont want to use these modifiers on cells like inside of the expandable content. This is what happened when I added those modifiers and we run into the problem where hiding the cell doesnt hide those styles that come with the expansion.
There was a problem hiding this comment.
nice, I'll look at that
There was a problem hiding this comment.
I tried this out and it seems to work OK for me. It may have been an issue with how it was written previously.
|
@mattnolting @christiemolloy updated. |
mattnolting
dismissed
their stale review
display property is still not correct, looking at a solution
|
the demo here looks good to me and I would like to prototype w/ the One additional thing I noted on visibility while looking at the w3 specs during the table's work so far is that we can use |
|
This is an example of hiding all of the
|
|
@christiemolloy this feature is for dropping columns. From #1725
So I would consider that a mis-use of the feature 😁 |
You could hide the entire row with the same method :D |
|
@mattnolting for sure, this feature is 🔥. Let's add that if we get a request for it. |
| tr:not(.pf-c-table__expandable-row) > :not(.pf-c-table__toggle):not(.pf-c-table__check):not(.pf-c-table__action):not(.pf-c-table__sort) { | ||
| [data-label] { | ||
| // default pf-hidden-visible() mixin is called in table.scss. redefining variable here | ||
| --pf-c-table-cell--hidden-visible--Display: var(--pf-c-table-cell--m-grid--hidden-visible--Display); |
|
🎉 This PR is included in version 2.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #1725
I had originally made this block of hidden/visible-on-* CSS a mixin that could be included in any component that takes an argument for the
displayproperty (forblock,table,flex, etc). That might be a good idea in the future if we want to add this kind of functionality to other components.