Adding a loading state on 'regio' page, and added some accessibility tweaks #4

JeroenReumkens · 2020-06-07T19:20:39Z

Hi there!

I really like to work that is being done on the dashboard. I took some time today to improve a few things I noticed, which because of the short timeline might not have made it into the first version yet. Let me know if you would like to merge these changes or have some feedback. I'd love to help, also with any other more pressing issues.

This PR has been split into #6 , #7 , #8 and #9 and will probably be closed.

~~The changes I made:~~

1. Added a small simple skeleton loading state for the graphs on the region page. That way the container doesn't jump around when the data is loaded.
2. Added screen reader only labels describing what the BarCharts show, for visually impaired users. (The content might need to be checked though).
3. Extended the graph titles on the region page with a (only for screen readers visible) text repeating the region this data is for.
4. Added some focus management which after selecting a region moves the focus to the heading of the first graph, so a user immediately can read the related data.
5. Added a screen reader only button at the end of the content bringing the user back to the region selector. This way a visually impaired user can switch easier between regions multiple times.

…is loaded, that way the content blocks don't jump around. Also added screenreader labels for the BarCharts, and added focus management when changing a region.

CLAassistant · 2020-06-07T19:20:43Z

All committers have signed the CLA.

JeroenReumkens · 2020-06-07T19:22:22Z

.prettierrc

@@ -0,0 +1 @@
+{ "singleQuote": true }


Added prettier because VSCode wanted to add double quotes. This way we ensure everyone uses the single quotes. Didn't seem to change any other code formatting in the files I touched.

We already provide a .prettierrc.js file with our configuration. There's no need to add this file.

If anything, we could take a look at automatically formatting files with prettier when committing them to the repo with a pre-commit hook. Is this something you'd like to investigate?

Is it than maybe possible that the .prettierrc.js file is in the private repo but not in the public one, since I don't see it?

As soon as I have the right config I will also add the precommit format, good idea! 👍

I have temporarily split this into a new issue: #7
In there you can hopefully help me find the prettier config, after which I or someone else can add it, together with the precommit hook.

JeroenReumkens · 2020-06-07T19:23:27Z

src/components/loadingPlaceholder/loadingPlaceholder.module.scss

+  background-color: $text-color-grey;
+  animation: loadingPlaceholderPulse 1200ms infinite;
+  display: block;
+  height: 80px;


Would could make this height a prop / inline style later if we needed different sizes. For now this is specific for the BartCharts. (Usually I use Styled Components for things like this, so I'm not sure if you would want to add an inline style if we'd made this dynamic)

For now this solution seems fine. I'd like to see 80px changed to 5em or 5rem depending on the parent font-size settings.

Fixed and created a new PR for only this change: #6

JeroenReumkens · 2020-06-07T19:24:36Z

src/utils/replaceVariablesInText.js

@@ -0,0 +1,28 @@
+const curlyBracketRegex = /\{\{(.+?)\}\}/g;
+


Added a very small utility to replace variables inside curly brackets. As far as I could see there wasn't anything in place for this yet? Tried to keep is as simple as possible for now.

It would be great if we can add some tests in Jest for utilities like these. A few basic assertions about input and expected output should be enough.

Created a todo in the new PR #8 which now contains the screenreader descriptions.
Will work on these unit tests ASAP.

VWSCoronaDashboard

Hi @JeroenReumkens

Thanks for helping out! We really appreciate it.

I left a few questions and comments as a review.

Besides those questions, could I ask you to split up the concerns in different PR's so we can perform our approvals/disapprovals on a more granular level? It would be a shame if we have to block a PR on minor disagreements on a small feature.

VWSCoronaDashboard · 2020-06-09T15:57:31Z

src/components/graphHeader/index.js

+      {/*
+        An optional ref and the tabIndex makes it focussable via JS,
+        this is used on the region page to focus the first graph header
+        after the region is changed.
+       */}
+      <h3 ref={headingRef} tabIndex={headingRef ? 0 : undefined}>
+        {title}
+        {regio && <ScreenReaderOnly>in {regio}</ScreenReaderOnly>}
+      </h3>


What is the default behavior without these changes? The focus stays on the last clicked anchor element right? Why is that bad?

Move to #9 and answered there.

VWSCoronaDashboard · 2020-06-09T15:57:57Z

src/components/loadingPlaceholder/index.js

@@ -0,0 +1,11 @@
+import styles from './loadingPlaceholder.module.scss';
+
+import * as React from 'react';


You don't need to import React in Next.js projects

Mind is blown. I work quite extensively with NextJS and have never known this 😄 It is fixed in the new PR.

VWSCoronaDashboard · 2020-06-09T16:00:30Z

src/utils/replaceVariablesInText.js

@@ -0,0 +1,28 @@
+const curlyBracketRegex = /\{\{(.+?)\}\}/g;
+


It would be great if we can add some tests in Jest for utilities like these. A few basic assertions about input and expected output should be enough.

VWSCoronaDashboard · 2020-06-09T16:02:20Z

src/pages/regio/index.js

+  /**
+   * Focuses the first heading in the content column,
+   * used after region changes to move focus for visually impaired
+   * users so they know what has changed.
+   */
+  const focusFirstHeading = () => {
+    try {
+      if (contentRef.current) contentRef.current.focus();
+    } catch {}
+  };


Why are you adding a try/catch without a catch?

Move to #9 and answered there.

JeroenReumkens · 2020-06-10T05:27:49Z

Hi @JeroenReumkens

Thanks for helping out! We really appreciate it.

I left a few questions and comments as a review.

Besides those questions, could I ask you to split up the concerns in different PR's so we can perform our approvals/disapprovals on a more granular level? It would be a shame if we have to block a PR on minor disagreements on a small feature.

Thanks for taking the time to review this!
I indeed got a bit carried away with multiple changes in 1 brach. Gonna try to create separate branches now and make PRs of these. I'll try to take in any changes based on the comments you made here, in those branches too.

JeroenReumkens · 2020-06-10T06:47:09Z

@VWSCoronaDashboard I have created new PR's ( #6 , #7 , #8 and #9 ), and will leave it up to you to close this issue as soon as you have read my replies.

Added a small loading indicator that is being shown when region data …

a45ff0d

…is loaded, that way the content blocks don't jump around. Also added screenreader labels for the BarCharts, and added focus management when changing a region.

JeroenReumkens commented Jun 7, 2020

View reviewed changes

VWSCoronaDashboard reviewed Jun 9, 2020

View reviewed changes

JeroenReumkens changed the title ~~Adding a loading state on 'regio' page, and added some accessibility tweaks~~ [WIP] Adding a loading state on 'regio' page, and added some accessibility tweaks Jun 10, 2020

JeroenReumkens changed the title ~~[WIP] Adding a loading state on 'regio' page, and added some accessibility tweaks~~ Adding a loading state on 'regio' page, and added some accessibility tweaks Jun 10, 2020

VWSCoronaDashboard closed this Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a loading state on 'regio' page, and added some accessibility tweaks #4

Adding a loading state on 'regio' page, and added some accessibility tweaks #4

JeroenReumkens commented Jun 7, 2020 •

edited

CLAassistant commented Jun 7, 2020 •

edited

JeroenReumkens Jun 7, 2020

VWSCoronaDashboard Jun 9, 2020

JeroenReumkens Jun 10, 2020

JeroenReumkens Jun 10, 2020

JeroenReumkens Jun 7, 2020

VWSCoronaDashboard Jun 9, 2020 •

edited

JeroenReumkens Jun 10, 2020

JeroenReumkens Jun 7, 2020

VWSCoronaDashboard Jun 9, 2020

JeroenReumkens Jun 10, 2020

VWSCoronaDashboard left a comment

VWSCoronaDashboard Jun 9, 2020

JeroenReumkens Jun 10, 2020

VWSCoronaDashboard Jun 9, 2020

JeroenReumkens Jun 10, 2020

VWSCoronaDashboard Jun 9, 2020

VWSCoronaDashboard Jun 9, 2020

JeroenReumkens Jun 10, 2020

JeroenReumkens commented Jun 10, 2020

JeroenReumkens commented Jun 10, 2020 •

edited

		@@ -0,0 +1,11 @@
		import styles from './loadingPlaceholder.module.scss';

		import * as React from 'react';

Adding a loading state on 'regio' page, and added some accessibility tweaks #4

Adding a loading state on 'regio' page, and added some accessibility tweaks #4

Conversation

JeroenReumkens commented Jun 7, 2020 • edited

CLAassistant commented Jun 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VWSCoronaDashboard Jun 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VWSCoronaDashboard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeroenReumkens commented Jun 10, 2020

JeroenReumkens commented Jun 10, 2020 • edited

JeroenReumkens commented Jun 7, 2020 •

edited

CLAassistant commented Jun 7, 2020 •

edited

VWSCoronaDashboard Jun 9, 2020 •

edited

JeroenReumkens commented Jun 10, 2020 •

edited