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

Adding unique IDs to EuiDatePicker Time component options. #5466

Merged
merged 5 commits into from
Dec 14, 2021
Merged

Adding unique IDs to EuiDatePicker Time component options. #5466

merged 5 commits into from
Dec 14, 2021

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Dec 13, 2021

UPDATE 12/14:
Question to reviewers below was spun out to a separate issue: #5471

Summary

axe-core was throwing an error for duplicated IDs in our Time component as part of EuiDatePicker. I added the htmlGenerator() function and appended a unique string for each LI. PR closes #5304.

Question to reviewers

In our legacy theme, the selected time always seems to be centered after click or keypress events. The Amsterdam theme doesn't always center the selected time. Is this expected behavior or a bug?

Screen Shot 2021-12-13 at 4 13 16 PM

Checklist

@1Copenut 1Copenut marked this pull request as ready for review December 13, 2021 22:17
@1Copenut 1Copenut changed the title Adding unique IDs to time picker options. Adding unique IDs to EuiDatePicker Time component options. Dec 13, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5466/

@cee-chen
Copy link
Contributor

In our legacy theme, the selected time always seems to be centered after click or keypress events. The Amsterdam theme doesn't always center the selected time. Is this expected behavior or a bug?

Sounds like a bug to me if it was working in legacy but not in Amsterdam - could you file a separate issue for this? (Or if you'd rather fix in this PR, feel free!)

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

LGTM - just one minor changelog suggestion

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Current code changes LGTM
Agreed that the Legacy-Amsterdam difference seems like a bug. Feel free to open an issue and circle back if you'd like.

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@1Copenut
Copy link
Contributor Author

Thank you EUI reviewers! I've spun out the Amsterdam CSS item as a separate issue so we can merge this PR. I'll pick it up as a fast follow item. #5471

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5466/

@1Copenut 1Copenut merged commit 1480a97 into elastic:main Dec 14, 2021
@1Copenut 1Copenut deleted the tpierce-eui-5304 branch December 14, 2021 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDatePicker][AXE-CORE]: Ensure IDs are unique for each time picker item
4 participants