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 table block header and footer toggles #15409

Merged
merged 13 commits into from May 9, 2019

Conversation

@talldan
Copy link
Contributor

commented May 3, 2019

Related issues: #15283, #6923

Description

Adds some toggle switches to allow a user to add and remove header and footer sections to the table block.

How has this been tested?

  1. Add a table block
  2. Specify the initial columns and rows
  3. Use the toggle switches in the sidebar to display and hide header and footer rows

Screenshots

Screen Shot 2019-05-03 at 2 56 20 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

This could definitely do with a design review. One area in particular would be making the table header and footer rows look nice. Especially with the striped style variation. Here's what it looks like with no styling:
Screen Shot 2019-05-03 at 3 01 40 pm

Something like this might be an improvement:
Screen Shot 2019-05-03 at 2 59 59 pm

(cc: @jasmussen - I remember you worked on stripes initially, let me know if you have any ideas on this 😄)

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Related issue: #15283

Authors using the visual editor can create a table, however the table
which results is only a series of rows and columns with no column-header
information, which may cause assistive technologies to assume these are
layout tables rather than data tables.

Perhaps given this information, the header section should be toggled on by default when creating a table?

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

This is what I'm seeing:

table

This isn't bad, actually!

The double borders beetween normal content and header or footer would be nice to not have. Not sure why they're present, but I can take a look if you need help.

I don't think we need to, or even should, do anything at all to style the table or footer sections. These are primarily there for semantics, and we should allow their default styles to shine through. I.e. bold headers, and the footers just look the same.

One question though is around flipping those switches. It's definitely boolean as it is now, which begs the question: is it ever okay to have two headers in a row? Or two footers in a row? O)r could you have a tablehead in the middle of a table? Because if that's semantically allowed, we'll need a different approach entirely :/

Inversely, if a tablehead can only semantically be at the top, and a table footer can only semantically be at the bottom, then the toggles are fine. Then the question just becomes whether we add those, or just convert the topmost table row to a table head.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Wow, thanks for that fast reply @jasmussen!

The double borders beetween normal content and header or footer would be nice to not have. Not sure why they're present, but I can take a look if you need help.

I added them thinking it improved things visually 😊 Pretty easy to remove them 😄

I don't think we need to, or even should, do anything at all to style the table or footer sections.

It's worth checking out the striped variation. At the moment the stripes don't quite line up when a header is added. Not sure how we could make that work with pure css.

One question though is around flipping those switches. It's definitely boolean as it is now, which begs the question: is it ever okay to have two headers in a row?

It is possible at the moment to add additional header/footer rows using the insert row option. I do have a concern that if the header/footer isn't presented differently it might be hard for the user to understand where they're inserting rows.

Or could you have a tablehead in the middle of a table?

At the moment I definitely feel it's sensible to keep it simple. The table block should still support other complicated tables if they're pasted, but until there are more advanced selection tools it might be best not to add too much functionality around modifying individual cells.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Yep, definitely good thoughts, and I share your desire both to keep things simple, and to ship improvements like this feature which is missing.

My question is purely one of valid HTML semantics, and perhaps in a way even accessibility: is the following markup okay?

<table>
    <thead>
...
    </thead>
    <tbody>
...
    </tbody>
    <thead>
...
    </thead>
    <tbody>
...
    </tbody>
</table>
```

If the above is not semantic/valid, it makes it a fair bit easier for us because those toggles can be completely fine for us even longterm. 

My concern is that if that markup is valid, a boolean choice is insufficient, and I'm worried about us moving too far down one path if we know at some point in the future we want to support something else. 
@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@jasmussen I think we're ok. According to MDN:

The <thead> must appear after any <caption> or <colgroup> element, even implicitly defined, but before any <tbody><tfoot> and <tr>element.

(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/thead)

And for tfoot:

The <tfoot> must appear after any <caption><colgroup><thead><tbody>, or <tr>element. Note that this is the requirement as of HTML5.
HTML 4 The <tfoot> element cannot be placed after any <tbody> and <tr> element. Note that this directly contradicts the above normative requirement from HTML5.

(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/tfoot)

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

That sounds very promising indeed! And it suggests this could be a fruitful path forward. 👍👍

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I've removed the borders for the header/footer rows for now.

@jasmussen jasmussen self-requested a review May 6, 2019

@jasmussen
Copy link
Contributor

left a comment

This is what I see now:

table

Honestly, I think given the semantics of the footer and header, this is a pretty elegant and good solution. It solves a problem, and as such, I'm just about ready to approve this.

But we need to think about how to deal with the striped variation:

stripes

Here's a design I think could work:

Screenshot 2019-05-06 at 09 01 17

The difference there:

  • No borders around the th elements
  • no backgrounds behind the header or the footer

Remove those, and this is good to go!

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@jasmussen Thanks for all the feedback. That's a nice and simple solution, I like it!

I've made those adjustments. Here's how it looks now with the Gutenberg Starter Theme:
Screen Shot 2019-05-07 at 5 38 28 pm

@jasmussen jasmussen self-requested a review May 7, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

I think that looks solid, especially given we mean to provide a baseline that themes can style further.

I checked this branch out intending to test it, but for whatever obscure reason I can no longer see the toggles in the sidebar. I am convinced that it is an error on my part, and ascribing it to me being up at 4:30am today with a screaming baby.

SO, judging purely by the screenshot, 👍 👍 from me. Would still love a sanity check from @kjellr, and a code review, but otherwise, good to go!

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Would still love a sanity check from @kjellr, and a code review, but otherwise, good to go!

Yep, that's working pretty well. I think we're good from a purely visual perspective. 👍

I do have one minor UX thing to note: If you enter text in a header or footer row and then toggle it off, your text disappears forever. I wonder if we should add a warning/confirmation in this case?

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I do have one minor UX thing to note: If you enter text in a header or footer row and then toggle it off, your text disappears forever. I wonder if we should add a warning/confirmation in this case?

Hey @kjellr 👋, thanks for the feedback. That's a good concern. Undo works in this case, but I don't think it's always apparent to a user in the moment that they can use undo.

I'll explore a few options around improving this.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Of those two, I think number 2 makes the most sense because it feels the most natural. But I also genuinely feel that such an effort should be made not on a per-block basis, but if possible, in a more generic way so that it would apply to all blocks that could do this, even third party blocks. I would go so far as to say that if we can't do that, we shouldn't do it at all.

Existing desktop apps also have this type of "lossy" behavior, and relies on undo as well.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

I wonder if it seems destructive because the toggle feels like a show/hide action, instead of the add/remove action that's actually happening.

I'll hold off pushing a solution, if there are some strong opinions that the action shouldn't be destructive I can look at pushing a commit with some caching of content.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

It's the same thing with the columns block. Create 3 columns, write something in column 3, resize in the sidebar to be 2 columns, and column 3 is lost. But it's so trivial to step back with the undo/redo buttons which are even surfaced visibly, that I honestly don't think it's a problem. It's the same in Keynote — if you use a template that has photo, title, body, fill those three out, and swap this to be a template that features only title and body, the photo is lost and does not come back when you swap back to the previous template. You just have to undo. It feels completely natural in practice.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Another possibility could be a confirmation message if the content is not empty when you untoggle

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

It feels like this is one of the places where we either trust our undo, or don't. If we trust it, we could also show a transient snackbar notice that you can undo: #13905 (comment)

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@jasmussen I don't disagree, just wanted to share another possibility :)

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

Oh I didn't want to come off as dismissive, I apologise if I did! Your suggestion reminded me of the snackbar notice which seems a perfect place to show little contextual tips like this!

@youknowriad
Copy link
Contributor

left a comment

Code wise, this looks good. Thanks for the improvement @talldan
An e2e test could be good addition :)

@kjellr

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

We probably don't want to implement some sort of warning when removing the existing content, but I think it's fine to hold off and address this holistically alongside the columns block. It shouldn't delay this merge in any case. This gets the 👍 from me as is. Thanks, everyone!

@talldan talldan force-pushed the add/table-block-header-footer-toggles branch from 0ee47a7 to f85a1c2 May 9, 2019

@talldan talldan requested a review from ntwb as a code owner May 9, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Thanks everyone for the help! E2E tests have been added, will merge once everything passes.

@talldan talldan merged commit 48c67fb into master May 9, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@talldan talldan deleted the add/table-block-header-footer-toggles branch May 9, 2019

@talldan talldan added this to the 5.7 (Gutenberg) milestone May 9, 2019

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.