-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat: add screen reader support to date headers #2493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, if you address the one item in the DateHeader.
Wait. I spoke too soon. What are you doing if it's not a drilldown?
src/DateHeader.js
Outdated
@@ -1,9 +1,18 @@ | |||
import PropTypes from 'prop-types' | |||
import React from 'react' | |||
|
|||
const DateHeader = ({ label, drilldownView, onDrillDown }) => { | |||
const DateHeader = ({ label, localizer, date, drilldownView, onDrillDown }) => { | |||
const renderLabel = () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing it this way recreates this function with each render of the DateHeader. Create a separate HeaderLabel component function above the DateHeader (you don't need to export it), passing it the label
, date
and localizer
, and use the component within the DateHeader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to move the label into a separate component.
@cutterbl sorry for the slow response, I seem to have stopped getting notifications when PR comments are made.
I'm not sure I understand this comment, in both cases the goal is to render the hidden |
</span> | ||
</> | ||
) | ||
const DateHeaderLabel = (label, localizer, date) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a true function component signature. The arguments are props, and should be destructured from the props object (see line 13 as an example).
When contributing changes like these, make sure that you're running Storybook locally (npm run storybook
). This will allow you to see your changes real time, and avoid errors.
Corrections, as described above, will apply to the 'day' display (the number in each day cell) of the Month view of the Calendar. For column headers (Month or TimeGrid views) you would update the Header component. In this case, it may be best to separate your DateHeaderLabel into it's own separate component, and import it into each file.
Further reflection on your 'format' shows that, while appropriate for the 'moment' localizer, it would break if someone was using the 'Luxon', 'dayjs' or the 'date-fns' localizers, all of which have different format tokens. I would suggest adding a new format type to each (screenReaderDateFormat
?) with the appropriate format, and then reference that in your new DateHeaderLabel
component localizer.format(date, 'screenReaderDateFormat')
.
Adds screen reader support to date headers, as discussed in #2488.