Skip to content

Conversation

@lyzadanger
Copy link
Contributor

This PR introduces a table pattern, and adjusts the scrollbox pattern to support it. It also (in a separate commit) makes a few adjustment to Library.Example and Library.Demo component layouts to give more options in how examples and demos are styled, giving more flexibility that was useful for this work.

This is part of #157

Following this will be a Table component implementation on top of this pattern (this is already in progress). I may also opt to create a simple Scrollbox container component to encapsulate some of the scrollbox pattern bits and bobs.

Updates to scrollbox pattern to support table

The recently-introduced scrollbox pattern is a CSS implementation of an overflow container with scrollability-hinting when that scrollable content is overflowing:

overflow-list-example

This PR contains a commit that adds some flexibility to the scrollbox pattern such that it can accommodate sticky headers, which are used in our table implementation. This variant (scrollbox--with-header) offsets the top hint-shadow down by one touch-target unit (44px in our implementation):

scrollbox--with-header

Without this affordance, tables will have bottom scroll-hint shadows, but not top ones, as those would be obscured by the sticky header background.

Aside about this positioning: This does require that our sticky headers used within such scrollboxes are one touch-target (44px) high. Which I grieved over slightly, but feel that the trade-off is reasonably OK. We at least have a common unit—that 44px touch minimum—to apply here. More importantly, heights might grow higher than the set height rule (e.g. text wrapping), but shouldn't get shorter. In the case that they grow higher, eh, no big deal, the top shadow will just be obscured (bottom shadow still works).

The table pattern

The table pattern implementation here is based on the table stying from the LMS project. It has been simplified to remove non-table concerns, adjusted and commented. The contrast felt just a tad high on the headers on LMS, so I've lightened that a touch, while still leaving the is-selected styling not at all subtle.

image

Partially-scrolled table inside of a scrollbox--with-header container:

image

Adjustments to Library Example and Demo layouts

There's a commit on this branch that adds a little layout flexibility to the Examples and Demos such that they don't always need to be 50%-50%, which is limiting in some cases. This allowed me to create a full-width table example in this case. This is just appearance stuff.

Try it out

Pull this branch, run your local webserver (make dev) and visit the Patterns: Containers page (for the scrollbox--with-header pattern example) and the Patterns: Tables page

Add "wide" variant option to `Library.Example` to allow for more
flexibility in example and demo layout. Remove extra "frame-i-ness" on
in demo appearance to reduce visual noise.
@lyzadanger lyzadanger added the pattern library concerning the build, structure, styling or components for the pattern library label Aug 9, 2021
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #168 (d79f1f7) into main (5934b67) will not change coverage.
The diff coverage is n/a.

❗ Current head d79f1f7 differs from pull request most recent head 83c5d6d. Consider uploading reports for the commit 83c5d6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #168   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          149       149           
  Branches        51        51           
=========================================
  Hits           149       149           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5934b67...83c5d6d. Read the comment docs.

@lyzadanger lyzadanger requested a review from esanzgar August 9, 2021 19:09
Copy link
Contributor

@esanzgar esanzgar left a comment

Choose a reason for hiding this comment

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

It looks good and behaves well.

Thanks for the hard work.

Comment on lines +288 to +289
className="hyp-u-layout-row--center hyp-u-border--bottom hyp-u-bg-color--grey-1"
style="position:sticky;top:0;min-height:44px;"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if having a .hyp-u-layout-table-header would help.

It could also include min-height: $touch-target-size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Tell you what. I was thinking of implementing a simple Scrollbox container component in my next body of work. I will look into introducing a pattern that corresponds to "a header of the correct size to put in a scrollbox" along the lines of what you're suggesting here. 👍🏻

<table className="hyp-table">
<thead>
<tr>
<th scope="col" style="width:30%">
Copy link
Contributor

Choose a reason for hiding this comment

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

I have never seen scope="col". (
TIL, according to MDN: If the scope attribute is not specified, or its value is not row, col, or rowgroup, or colgroup, then browsers automatically select the set of cells to which the header cell applies.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I also just TIL that particular detail. From the styling side, the table-layout: fixed rule in the SASS makes it so that the table and column width is derived from the first row in the table, making it possible to style the th widths and have that reliably apply to the whole table.

className="hyp-scrollbox--with-header"
>
<table className="hyp-table">
<thead>
Copy link
Contributor

Choose a reason for hiding this comment

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

thead has a min-height of touch $touch-target-size, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, a height rule that behaves as a min-height, as min-height doesn't apply for elements with display: table-cell. But, yes, in concept.

@lyzadanger lyzadanger merged commit 73494ad into main Aug 10, 2021
@lyzadanger lyzadanger deleted the table-pattern branch August 10, 2021 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pattern library concerning the build, structure, styling or components for the pattern library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants