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

Infographics and tables #107

Merged
merged 18 commits into from
Jun 17, 2019
Merged

Infographics and tables #107

merged 18 commits into from
Jun 17, 2019

Conversation

Heydon
Copy link
Contributor

@Heydon Heydon commented Jun 7, 2019

Includes both (interrelated) components, and their write-ups.

@Heydon Heydon requested a review from micmath June 7, 2019 15:16
@simonsinclair simonsinclair self-requested a review June 10, 2019 13:06
@simonsinclair
Copy link
Contributor

simonsinclair commented Jun 10, 2019

Hi Heydon! @micmath asked me to look at this. I've just loaded it up locally and had a play:

Reference implementation - Data Tables

  • Works well in Chrome and Firefox on Mobile and Desktop with or without JS enabled.
  • Appears to have a display bug in Safari. I took a screenshot.
  • Resides under demos/tables/, but other components follow a pattern of demos/article-name/. Should this be demos/data-tables/?
  • Has a potential UX/usability concern with regards to the visual state of the sorting buttons not changing when pressed.

Article - Data Tables

  • Doesn't appear to feature on the GEF Docs index.
  • Reads well.

I hope this helps.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 11, 2019

@simonsinclair Hi Simon! Thanks for taking a look.

Did you only try the table reference implementation, and not the infographics one?

Appears to have a display bug in Safari. I took a screenshot.

Damned Safari! It was a bug with transparent (which Safari interprets as grey!!!???). Fixed.

Resides under demos/tables/, but other components follow a pattern of demos/article-name/. Should this be demos/data-tables/?

Good spot! Done.

Has a potential UX/usability concern with regards to the visual state of the sorting buttons not changing when pressed.

So... in research (not at the BBC), it was found that folks weren't sure whether buttons should display the current state or the state to be attained after the next button press. That is, changes in state were confusing. For now, I have the buttons just say "pressing this will sort the column" to avoid the state complexity issues. The change in state is visible simply because the column is seen to sort + ARIA is provided on the parent <th> (e.g. aria-sort="ascending"). I'm happy with this for now.

@simonsinclair
Copy link
Contributor

simonsinclair commented Jun 11, 2019

Morning,

I just reviewed Infographics. (Why am I not seeing either of these articles on the GEF Index?)

Reference Implementation - Infographics

  • Works great in Chrome and FF, but
  • there's a similar bug to Data Tables in Safari.

Article - Infographics

  • There seems to be a stray key-line in Chrome. See screenshot.
  • You link to components/tables in "The source data can be presented in tabular form, using a table component:" which 404's.
  • Reads well!
Copy link
Contributor

@micmath micmath left a comment

Choose a reason for hiding this comment

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

This is really great @Heydon! I've noted a few minor tweaks, but am loving the information! 😍

```

To make this element scrollable by keyboard, it must first be focusable. This requires the `tabindex="0"` attribution. For screen reader users, this newly interactive element will need a label. It's recommended the element takes the `group` role and is associated with the `<caption>` for the labeling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe "labeling" is the American spelling, "labelling" is the British spelling. We should always use the British spellings.

* Is scalable without degradation, facilitating zoom/magnification
* Can be highly optimized, especially when using the [simple shapes and paths recommended](https://www.bbc.co.uk/gel/guidelines/how-to-design-infographics#infographic-visual-style-overview)
* Can be maintained and updated easily, like any other markup
* Unlike runtime-dependent `<canvas>` graphics, can contain accessibility information, and be downloaded as a self-sufficient asset
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean "self-contained" asset?


Visual and figurative representations of information can help readers to quickly grasp the salient points. However, without careful and deliberate design they can be confusing, or even misleading.

The visual design and labeling of infographics (or 'visualisations') is [covered comprehensively in GEL](https://www.bbc.co.uk/gel/guidelines/how-to-design-infographics). As a complement to the GEL documentation, here we shall explore two topics:
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer British spelling "labelling".

* Unlike runtime-dependent `<canvas>` graphics, can contain accessibility information, and be downloaded as a self-sufficient asset

Inline SVG (SVG embedded directly into the HTML) is the most malleable, since you can target individual SVG elements/artifacts with CSS and JavaScript. Embedding SVG also reduces http requests and, most importantly, _content reflows_[^2], by loading and rendering along with the surrounding content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer British spelling "artefacts".

Copy link
Contributor

Choose a reason for hiding this comment

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

There are honest differences of opinions about whether image assets in general should be served via separate HTTP requests, because those requests can be given different caching policies than the text, for example an image could have a 10-year publicly cachable policy based on the filename, whereas a personalised page migh need a no-cache policy. So this argument is nuanced and highly dependent on workflow, content and traffic patterns. I'd recommend sidestepping that debate by not raising the "reduces http requests" angle; The stronger usability argument is that it avoids reflows. In addition, even if a team has strictly separate workflows which demand that designers create images and coders create pages, it's fair to say that data visualisations are a special exception because they are not purely images (they are also data) so they belong with the text.


### Labels and descriptions

_Every_ infographic should be provided with alternative text (an accessible label) to describe it to the visually impaired. In addition, _some_ infographics should be accompanied by a caption. The role of the alternative text and the role of the caption are distinct. The alternative text _describes_, and the caption _explains_:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can include a ref or link to an "alternative text" guideline here?


### Column headers

Any `<table>` that does not have column or row headers (`<th>` elements) cannot be considered a true data table. In fact, some screen readers that encounter a table without headers will determine it as a 'layout table' and communicate it quite differently[^1].
Copy link
Contributor

Choose a reason for hiding this comment

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

Reads better? "will treat it as a 'layout table'"

Note that the `<table>` is now divided into a head (`<thead>`) and body (`<tbody>`). This does not have any impact on the behavior of the table and its headers. But, as we enhance the table, these elements will come in useful for styling and scripting.

::: alert Empty table headers
In the last example, the first column header is effectively a header for the _row_ headers. Some deem this unnecessary and make the cell empty. This is not recommended, since it can produce unexpected behavior[^2]. It is also better to be explicit whenever there is the opportunity. In the [**Reference implementation**](#reference-implementation) the row headers are each premier league football teams. This could probably be concluded through deduction (and the help of surrounding information), but an explicit _"Team"_ column header removes all doubt.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you mean the Premier League, it should be capitalised?


### Captions

So far, we have labeled the rows and columns, but not the table itself. You may be inclined to introduce a table with a heading element, such as an `<h2>`. This would certainly aid users browsing the page visually. However, a heading would not be directly _associated_ with the table, meaning a screen reader user navigating directly to the table (using a shortcut like <kbd>T</kbd> in NVDA) would not hear a label announced upon arrival.
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer British spelling "labelled".

## Recommended layout

Importantly, the grid structure of data tables must remain intact no matter the available space. That is, elements must not wrap or otherwise change position since they will become labeled incorrectly. For tables with many columns this may precipitate horizontal scrolling. It is recommended a container element with `overflow-x: auto` is used to contain the horizontal scroll behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reads better? "this may result in horizontal scrolling"

```

::: alert Conditional interactivity
It is not recommended the table container is made focusable by default, since tables that do not trigger a scrollbar would result in an interactive container that _doesn't do anything_. In the [**Reference implementation**](#reference-implementation) `tabindex="0"` is removed via `ResizeObserver` where the container is not overflowing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reads better? "where the container does not overflow"

Copy link
Contributor

@simonsinclair simonsinclair left a comment

Choose a reason for hiding this comment

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

https://github.com/bbc/gef-docs/blob/55a8738251c35a5d7d3a549454bda3775f47d4c5/docs/components/demos/data-table/index.html#L43-L51 I think your fingers might need oiling, @Heydon. 😄

Other than that, all's functioning well my side. It's awesome.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 12, 2019

@micmath @simonsinclair Haha, okay. Thanks, both — I've fixed these.

@micmath
Copy link
Contributor

micmath commented Jun 14, 2019

@Heydon just waiting for you to resolve the remaining change requests (happy to debate any of them you think I might have got wrong!)

Also, it seems like the sticky table-of-contents was disabled in this PR? We tried to replicate the UX of the TOC seen here: https://www.bbc.co.uk/gel/guidelines/how-to-design-forms . Was the sticky behaviour removed intentionally? Can we get it back?

@Heydon
Copy link
Contributor Author

Heydon commented Jun 14, 2019

@micmath Sorry, Michael. Which change requests are you referring to? I did the ones noted above (or did they not appear?). The sticky thing was removed as it was affecting the data tables, if you recall. However, I can reinstate that by using a different classname than sticky it would seem.

@micmath
Copy link
Contributor

micmath commented Jun 14, 2019

@Heydon my github view of the list above shows the following are currently unchanged from the review:

  • src/components/data-tables.md#126
  • src/components/infographics.md#14
  • src/components/infographics.md#28
  • src/components/infographics.md#32
  • src/components/infographics.md#170
  • src/components/infographics.md#174
  • src/components/data-tables.md#13
  • src/components/data-tables.md#118

it might be my own reading-fail but I'd be grateful if you could reconfirm you've updated those.

Re the sticky TOC, my understanding was that you were planning to reimplement that in pure CSS, and thus wouldn't need the JS to make it work. As the PR currently stands it just doesn't work fullstop(?), so I suppose I'm asking about tha: Is it the case that the DataTables example cannot work on a page with a sticky TOC? If so I'd like to find a solution that doesn't require disabling it if possible? But I defer to you as you know more about the issues.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 14, 2019

@micmath Your suggestions were spot on and I changed the code but it may not have pushed. Just given my laptop to a conf for my talk in a bot, so this may have to be tomorrow now, but I will check this and make sure the sticky stuff is reinstated without breaking my tables.

@micmath
Copy link
Contributor

micmath commented Jun 14, 2019

Awesome stuff, sir. 💃

@Heydon
Copy link
Contributor Author

Heydon commented Jun 15, 2019

@micmath I believe this is okay now. Sticky menu reinstated, with sticky tables working separately. Also, went over the other bits. I missed one in the infographics file which I've now caught. Plus: alt text reference, and rejigged footnotes.

@micmath
Copy link
Contributor

micmath commented Jun 17, 2019

I pulled the latest changes to review, but there were a couple of issues that jumped out:

I assume you just dropped a copy-past somewhere for the introduction. I'll investigate the weird grey line, but possibly you are more familiar with the CSS and can spot the problem faster than me?

@Heydon
Copy link
Contributor Author

Heydon commented Jun 17, 2019

@micmath You are quite right. I'll write that introduction now.

@micmath
Copy link
Contributor

micmath commented Jun 17, 2019

The plot thickens. The stray line seems to be related to the css for the table. Can you hunt that bug?

@Heydon
Copy link
Contributor Author

Heydon commented Jun 17, 2019

@micmath Just pushed the intro.
Sure! It's almost certainly a dropped position: relative.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 17, 2019

@micmath Fixed and pushed. Basically, the stickiness results in no bottom border for the thead column headers. So I nneded to reinstate it using pseudo-content.

@micmath micmath merged commit f1b0876 into master Jun 17, 2019
@micmath
Copy link
Contributor

micmath commented Jun 17, 2019

Thank you!

@micmath micmath deleted the viz branch June 17, 2019 10:38
@micmath
Copy link
Contributor

micmath commented Jun 17, 2019

These are now live,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants