Feature/accessibility improvements#2
Conversation
It doesn't appear necessary to specifies the privileged IP's in order to run this project locally. Defaulting to an empty string to prevent null point exception
I don't see any scss -> css build steps. I feel like I must be overlooking something but this is an easy fix for now.
| node_modules | ||
| css | ||
| node_modules-2018-11-27 | ||
| .idea |
There was a problem hiding this comment.
I use intelliJ, this is to prevent my IDE files from being committed
| @@ -1,5 +1,5 @@ | |||
| <!doctype html> | |||
| <html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
Adding the lang helps non-human readers to identify what language is to be expected from this page.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/lang
| "watch": "watchify index.js -p livereactload -o bundle.js -dv", | ||
| "watchServer": "node-supervisor --watch server.js,src server.babel.js", | ||
| "serve": "PORT=1314 node server.babel.js", | ||
| "build-css": "sass ./sass/main.scss ./css/main.css", |
There was a problem hiding this comment.
I didn't see any build process for CSS. I've added these scripts and updated the documentation to call it out. Let me know if I overlooked the build process somewhere however.
| "watchify": "^3.9.0" | ||
| }, | ||
| "dependencies": { | ||
| "sass": "^1.24.0", |
There was a problem hiding this comment.
sass dependency for the css build scripts: https://github.com/nlaffey
| @@ -1,4 +1,10 @@ | |||
| /* reset & clearfix */ | |||
| /* reset & clearfix | |||
There was a problem hiding this comment.
Per my comments here, I've removed the outline styles from the reset. This is something that I would want to run by a designer to see if its suitable for the site. It's important to keep in mind each browser will implement the focus style differently but this may be for the best. Users will be most familiar with their browsers focus style.
| app.use((req, res, next) => { | ||
| var actingUrl = req.url.replace(/\/(.*)\/$/, '/$1') | ||
| const internalIPs = process.env.PRIVILEGED_IP_LIST | ||
| const internalIPs = process.env.PRIVILEGED_IP_LIST || ''; |
There was a problem hiding this comment.
Rather than force users to provide the environment variable I thought it made sense to default to an empty string. If this is set to null then it throws a null pointer error below when we try to .split
| <Peek facet="department" q={deptName} quiltProps={{maxRowHeight: 400}} offset={10}/> | ||
| <Helmet title={deptName} /> | ||
| <Departments compact={true} active={deptName} /> | ||
| <main role="main" className="departmentPage" id="main"> |
There was a problem hiding this comment.
By using the main element we help tell screenreaders/search engines where the primary content is. This can help consumers more quickly locate the unique content of this page rather than scaffolding around it (footer, navigation, etc).
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles
|
|
||
| render() { | ||
| return <div className="landingPageBody"> | ||
| return <main role="main" id="main" className="landingPageBody"> |
There was a problem hiding this comment.
Same concept as here: https://github.com/nlaffey/art/pull/2/files#r361749695
There's also likely other routes on the site that could use this same treatment.
| }; | ||
|
|
||
| return <Link | ||
| onKeyDown={handleKeyDown} |
There was a problem hiding this comment.
Enter key presses were not registering as clicks. You could tab to the image but you weren't able to access it without using the mouse. This allows the enter key to be pressed.
| render() { | ||
| var {number, prevLink, nextLink, ...extraProps} = this.props | ||
| var thumbnail = <img | ||
| // TODO: Update to pass in an appropriate description of the image instead of just the number |
There was a problem hiding this comment.
Unfortunately there didn't seem to be a quick way to get the alt text here. Generally I feel that some alt text is better than nothing.
| const formProps = universal ? {action: "/search/", method: "get"} : {action: ''} | ||
| const simpleSearchBox = <div className='search-wrapper' style={{width: idealSearchBoxWidth + 'em'}}> | ||
| <form {...formProps}><input className='search-input' type="search" | ||
| <form {...formProps} role="search"> |
There was a problem hiding this comment.
Adding role of search here to help identify what this form is being used for
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Search_role
| <form {...formProps}><input className='search-input' type="search" | ||
| <form {...formProps} role="search"> | ||
|
|
||
| <label htmlFor='search-input' className="visuallyhidden">Search</label> |
There was a problem hiding this comment.
Adding a label for this input both using arial-label as well as a label that is visuallyhidden (this uses material-ui's class to marginLeft it off the screen)
| @@ -0,0 +1,10 @@ | |||
| var React = require('react'); | |||
|
|
|||
| var SkipToMainContentLink = React.createClass({ | |||
There was a problem hiding this comment.
I created this class as I was originally going to put the skip to main content link in a few different places. Ultimately I only ended up using it one place but decided to leave it here in it's own class for better re-usability.
There was a problem hiding this comment.
By adding the skip link it helps users quickly navigate over tedious navigation links so they can go straight to the main content of the page. The link is only visible when focused so only users with screen readers or tabbing through the page would reveal it:
1 participant
See the google doc for an overview of the project: https://docs.google.com/document/d/1MxWJfqjMd25K7_YY8hIiLPtO2iXiB9n6Zx9FXK_a8M0/edit?usp=sharing