-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add striped table variation #10428
Add striped table variation #10428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coolio!
@@ -89,6 +89,11 @@ export const settings = { | |||
foot: getTableSectionAttributeSchema( 'foot' ), | |||
}, | |||
|
|||
styles: [ | |||
{ name: 'default', label: __( 'Regular' ), isDefault: true }, | |||
{ name: 'striped', label: __( 'Striped' ) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this as "Stripped", I wonder if "Stripes" is more obvious? I dunno. Both are kinda weird. Whatever! 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to any verbiage, but definitely good to change now rather than later.
I actually intended to call this "zebra striped", but just striped seemed better based on a Google search.
This reminds me, I meant to ask @youknowriad: can we collapse the alignments for the table block by default no matter the breakpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, with the way they're defined in this block (hook triggered by supports: { align: true }
) it's not possible to do so without impact on all the other blocks using this hook. I think I'm fine duplicating the alignment hook in the block (I don't like this support key) but I know @jorgefilipecosta was doing some work on that, so he might know more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Stripes" looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design wise approving this. Looks great and nice addition!
Cool, I'll change to "stripes" and merge. |
Should this be the default? |
It makes it easier to follow large amounts of tabular data. I would be for stripes being the default.
|
Cool! I've also got a branch for changing background/text color that I think is almost ready, and I should be able to get it to work with this. (I'm imagining changing background color will only apply to the striped row, that's probably the simplest option to begin with). |
7a2c8dd
to
d141335
Compare
Changed this to "stripes". But wow, such a positive reception to this variation, and a few thumbs up to making it default. Although I like the striped variation, I have a slight preference for the bordered one being default, for two reasons:
See GIF for 2: By making the stripes a non-default variation the user can still choose this, but it's more intentional and part of a journey which should help inform that it's still a table. Thoughts? |
Would it be too confusing to have borders for either:
If they're both bad ideas then it not being the default is fair, but I feel like we could improve the editing UX by making it a bit different than the frontend, if that won't be too unexpected 🤷♂️ |
I do think it makes sense to look at UX improvements for the editing interface, for example showing the borders of the block when the table block is selected, but not when unselected. But I'm not totally sure how to approach this in a way that wouldn't balloon this PR. Any ideas on how to do this in a simple way? If so feel free to push! |
Let's get this in, we can revisit the default later. It would also be cool to add color support and use it for the stripes at some point. |
This PR adds a simple striped variation of the table. It features a light gray background on odd rows, and a thin bottom border in case the last row is even (and therefore without a boundary because of hidden cell borders).
d141335
to
6ea7f3c
Compare
Rebased. |
Happy for this to be merged @jasmussen? It looks like all the reviewers are happy. |
Yep, was waiting for 4.0 rc. |
Would be nice to have thead option because I work with tables a lot and use thead/th |
This PR adds a simple striped variation of the table.
It features a light gray background on odd rows, and a thin bottom border in case the last row is even (and therefore without a boundary because of hidden cell borders).
Default table:
Variation:
Theme: