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

Add striped table variation #10428

Merged
merged 3 commits into from Oct 11, 2018
Merged

Add striped table variation #10428

merged 3 commits into from Oct 11, 2018

Conversation

jasmussen
Copy link
Contributor

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:

screenshot 2018-10-09 at 10 33 32

Variation:

screenshot 2018-10-09 at 10 39 22

Theme:

screenshot 2018-10-09 at 10 40 50

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@jasmussen jasmussen self-assigned this Oct 9, 2018
@jasmussen jasmussen requested a review from a team October 9, 2018 08:41
Copy link
Member

@tofumatt tofumatt left a 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' ) },
Copy link
Member

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! 🤷‍♂️

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Member

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.

@tofumatt tofumatt added this to the 4.1 milestone Oct 9, 2018
@karmatosed karmatosed self-requested a review October 9, 2018 16:58
Copy link
Member

@karmatosed karmatosed left a 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!

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@jasmussen
Copy link
Contributor Author

Cool, I'll change to "stripes" and merge.

@mtias
Copy link
Member

mtias commented Oct 9, 2018

Should this be the default?

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label Oct 9, 2018
@tofumatt
Copy link
Member

tofumatt commented Oct 9, 2018 via email

@talldan
Copy link
Contributor

talldan commented Oct 10, 2018

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

@jasmussen jasmussen force-pushed the try/table-zebra-style branch 2 times, most recently from 7a2c8dd to d141335 Compare October 10, 2018 07:48
@jasmussen
Copy link
Contributor Author

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:

  1. It's what tables look like most of the time
  2. It's slightly more apparent where to click when the tablecells are empty

See GIF for 2:

table

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?

@tofumatt
Copy link
Member

Would it be too confusing to have borders for either:

  1. borders for all "stripes" tables only in the editor but not on the front-end (Gutenberg is not WYSIWYG)
  2. borders for empty stripes tables

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 🤷‍♂️

@jasmussen
Copy link
Contributor Author

borders for all "stripes" tables only in the editor but not on the front-end (Gutenberg is not WYSIWYG)

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!

@mtias
Copy link
Member

mtias commented Oct 10, 2018

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).
@jasmussen
Copy link
Contributor Author

Rebased.

@talldan
Copy link
Contributor

talldan commented Oct 11, 2018

Happy for this to be merged @jasmussen? It looks like all the reviewers are happy.

@jasmussen jasmussen merged commit f2da3e5 into master Oct 11, 2018
@jasmussen jasmussen deleted the try/table-zebra-style branch October 11, 2018 04:20
@jasmussen
Copy link
Contributor Author

Yep, was waiting for 4.0 rc.

@Mocha365
Copy link

Would be nice to have thead option because I work with tables a lot and use thead/th

@Soean
Copy link
Member

Soean commented Oct 22, 2018

@Mocha365 See #6923

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants