Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add custom ID to grid elements #1730

Merged
merged 9 commits into from
Jun 19, 2023
Merged

Conversation

GeorgeTaveras1231
Copy link
Contributor

Context

This allows creating combobox elements that are properly linked via aria-controls

Resources:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/combobox_role

Analysis

Currently, there is no id set on the grid element, and there isn't a way to set it.

Solution

Leverage existing pattern for adding IDs to caption and aria-describedby property.

I left a comment because to a reader of the code, it may seem like the ID serves no purpose (since it isn't used internally).

nitpick: I don't like that we unconditionally add an ID here, even though it is probably harmless, its not needed by most consumers, and has the side-effect of taking a naming in the global id namespace. Interested in feedback if there is a simple/elegant path for conditionally setting this id.

@GeorgeTaveras1231
Copy link
Contributor Author

@gpbl any interest in merging this or improving it?

@gpbl
Copy link
Owner

gpbl commented Apr 8, 2023

Thanks @GeorgeTaveras1231 yes interested to merge it! Thanks for your contribution!

Could you share some more complete code showing how to use this id? The one I find in the comment is not clear:

<input role="combobox" aria-haspopup="grid" aria-controls="daypicker-id-0-grid" />

DayPicker can render multiple months (grids). Why should an input control the first grid and not the whole calendar – what about the other grids?

Would something like this work better?

<input role="combobox" aria-haspopup="grid" aria-controls="testId" />
<DayPicker id={testId} />

@gpbl gpbl changed the title feat: add ID to grid element feat: add custom ID to grid elements Apr 8, 2023
@gpbl gpbl added the need more info Lacks enough details to make progress label Apr 16, 2023
@GeorgeTaveras1231
Copy link
Contributor Author

Thanks @GeorgeTaveras1231 yes interested to merge it! Thanks for your contribution!

Could you share some more complete code showing how to use this id? The one I find in the comment is not clear:

<input role="combobox" aria-haspopup="grid" aria-controls="daypicker-id-0-grid" />

DayPicker can render multiple months (grids). Why should an input control the first grid and not the whole calendar – what about the other grids?

Would something like this work better?

<input role="combobox" aria-haspopup="grid" aria-controls="testId" />
<DayPicker id={testId} />

@gpbl

I think your example may have an error because the testId is passed as a string to aria-controls and as a variable to DayPicker.

And yea, well, the input element's aria-controls has to equal a space delimited list of all the grid elements. In the case where multiple months are rendered, the caller can explicitly pass 2 or more IDs following the naming convention implemented by the current pattern.

Examples: Single month

<>
  <input role="combobox" aria-haspopup="grid" aria-controls="daypicker-id-0-grid" />
  <DayPicker id="daypicker-id" />
</>

Examples: Multiple months

<>
  <input role="combobox" aria-haspopup="grid" aria-controls="daypicker-id-0-grid daypicker-id-1-grid" />
  <DayPicker id="daypicker-id" numberOfMonths={2} />
</>

@GeorgeTaveras1231
Copy link
Contributor Author

@gpbl just a status update on this. Was my explanation clear and sufficient? Is there anything missing/unclear in the example?

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgeTaveras1231 ! I understand the use case now. I'm OK adding an id attribute to the rendered grid.

I think your implementation could be simplified if we use year and month to create the unique id, as opposed to the use of displayIndex.

PS. could you yarn lint --fix before pushing? Thanks!

@gpbl gpbl removed the need more info Lacks enough details to make progress label May 26, 2023
Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

@GeorgeTaveras1231 would you address my changes so we can move forward? Thanks a lot for your help!

@@ -50,7 +61,7 @@ export function Month(props: MonthProps) {
return (
<div key={props.displayIndex} className={className.join(' ')} style={style}>
<CaptionComponent id={captionId} displayMonth={props.displayMonth} />
<Table aria-labelledby={captionId} displayMonth={props.displayMonth} />
<Table id={tableId} aria-labelledby={captionId} displayMonth={props.displayMonth} />
Copy link
Owner

Choose a reason for hiding this comment

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

This line doesn't pass the linter - could you run yarn lint --fix to fix the linter issues? Thanks.

@GeorgeTaveras1231
Copy link
Contributor Author

@gpbl I've made the updates you've requested. Let me know if there's anything else I should do.

@gpbl gpbl merged commit 8ac42a5 into gpbl:main Jun 19, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants