Ensure unique ID in combobox with prepended or appended labels by 1Copenut · Pull Request #5229 · elastic/eui

1Copenut · 2021-09-30T19:56:22Z

Summary

I discovered a unique ID was not being passed from the parent EuiCombobox component to a child label when passing the prepend or append props. This was causing a semantic break between the label and input for screen readers and throwing an axe violation. This PR fixes #5183 for the axe violation.

Made the following changes:

Added a call to ensure a unique ID is generated if users do not pass their own ID to an instance of EuiCombobox
~~Updated the visual styling to include our blue or red lower border in the prepended and appended labels~~
Updated the visual styling to round the right edge of our input so it didn't break the right border
Added an example prepended combobox to our documentation with code snippet

Checklist

Visual Testing Matrix

After a couple of rounds of missed visual changes, I got organized and created an organized list of visual tests.

0 toggle is turned off
1 toggle is turned on

Theme	Full Width	Compressed	Prepended	Appended	Passed?
Control	0	0	0	0	✅
Amsterdam Light	1	0	0	0	✅
Amsterdam Light	0	1	0	0	✅
Amsterdam Light	0	0	1	0	✅
Amsterdam Light	0	0	0	1	✅
Amsterdam Light	1	0	0	1	✅
Amsterdam Light	0	1	0	1	✅
Amsterdam Light	0	0	1	1	✅
Amsterdam Light	1	0	1	1	✅
Amsterdam Light	0	1	1	1	✅
Amsterdam Light	1	1	1	1	✅

I then repeated this process for Amsterdam Dark, Light, and Dark themes. All ten combinations looked correct in all four themes.

* Checking for a user-passed ID prop * If no user-passed ID, we'll use the `rootId()` class method to set one * Prepend and append props need an ID to relate label to the input

* Confirmed the label `for` attribute is referencing input `id` * Added styles to round the edges of the child `<div>` when we prepend a label * Bumped package.json

Master merge from Elastic EUI upstream

cchaos

Thanks for fixing those pesky border radius'! ❤️

I know this is still draft, but the one thing I am going to push back on, stylistically, is the expansion of the focus/invalid borders into these prepend/append labels. Mainly I'm thinking about consistency with our other inputs.

I'm not fully opposed to the idea, but let's not bundle that into this PR, but open a design discussion around it. The few other things that need to be considered here are when it's not just text/label.

You can find a large example of different types of prepend/append content and combinations here: https://elastic.github.io/eui/#/forms/form-controls#prepend-and-append

src/themes/eui-amsterdam/overrides/_form_control_layout.scss

kibanamachine · 2021-09-30T21:00:42Z

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

cee-chen

Woohoo, first PR!! 🎉

I mostly just looked at the JS - just suggestions there, no blocking comments - and left a passing comment on the changelog entry. I'll leave the CSS and documentation feedback to @cchaos who is way more of an expert than I am!

src/components/combo_box/combo_box.tsx

src/components/combo_box/combo_box_input/combo_box_input.tsx

CHANGELOG.md

* Simplified the Amsterdam overrides for prepended / appended labels * Added conditional classes to `combo_box.tsx` * Added unit tests for user passed ID, prepended and appended classes

kibanamachine · 2021-10-01T15:57:12Z

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

cchaos

We seemed to have forgotten to add the prepend option to the display toggles in the example prior. Can you add canPrepend as an option to the wrapping <DisplayToggles> in that demo?

src-docs/src/views/combo_box/combo_box_example.js

src/themes/eui-amsterdam/overrides/_form_control_layout.scss

kibanamachine · 2021-10-01T17:53:05Z

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

* Added CSS rules for compressed, prepend and append border radius * Updated the Display Toggles to include append * Removed a duplicated instruction paragraph * Simplified the logic for a unique ID in combobox.ts

kibanamachine · 2021-10-11T17:04:28Z

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

1Copenut · 2021-10-13T14:44:31Z

src/themes/eui-amsterdam/overrides/_form_control_layout.scss

@@ -1,3 +1,37 @@
+.euiComboBox {
+  // Round the left border when we append a label
+  &--appended {


@cchaos I found a better way to isolate these rounded corners from your original comment, and was able to include the smaller rounded border for compressed views.

😬 Looks like still an issue when both are exist. lol

That it is. I didn't anticipate this scenario, so I'll pick it up in the morning for another look. Thanks Caroline!

@cchaos I've posted a fix and added a Visual Testing Matrix comment to the description. I think we should have them all bracketed this time.

* The borders were still rounded with prepend and append enabled * Added two more rules to make borders square for full height and compressed

kibanamachine · 2021-10-14T16:29:56Z

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

1Copenut · 2021-10-18T19:43:50Z

@cchaos CSS has been updated and should be 💯 now. Thank you!

cchaos

I pushed a quick fix that moved the combo box specific styles into it's own file. LGTM! Just needs a rebase with master and to move the CL entry up to the new section.

kibanamachine · 2021-10-18T20:25:03Z

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

thompsongl

Code changes LGTM 👍

…evorp_combobox_id

kibanamachine · 2021-10-19T17:05:38Z

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

…ic#5229) * Making an explicit check for ID prop in EUI Combobox * Checking for a user-passed ID prop * If no user-passed ID, we'll use the `rootId()` class method to set one * Prepend and append props need an ID to relate label to the input * Adding example for Combobox with prepended label * Confirmed the label `for` attribute is referencing input `id` * Added styles to round the edges of the child `<div>` when we prepend a label * Bumped package.json * Adding CHANGELOG entry and bumping version * Update combo_box.tsx * Updating styles to match other forms with prepend and apppend. * Finishing Combobox CSS and test updates * Simplified the Amsterdam overrides for prepended / appended labels * Added conditional classes to `combo_box.tsx` * Added unit tests for user passed ID, prepended and appended classes * Reordering props in child component. * Final adjustment to ID check in ComboBox * Final updates to styling and documentation * Added CSS rules for compressed, prepend and append border radius * Updated the Display Toggles to include append * Removed a duplicated instruction paragraph * Simplified the logic for a unique ID in combobox.ts * Adding Combobox unique ID to master changelog * Updated Form Control Layout SCSS * The borders were still rounded with prepend and append enabled * Added two more rules to make borders square for full height and compressed * Move to _combo_box.scss Co-authored-by: cchaos <caroline.horn@elastic.co>

@see

* Provide rows for datagrid cells to be owned by * changelog * undoing things * working except virtualized container * working row wrapper implementation * Create datagrid row elements on-demand and render cells via portals * Provide rows for datagrid cells to be owned by * changelog * undoing things * working except virtualized container * working row wrapper implementation * Create datagrid row elements on-demand and render cells via portals * Small style cleanup * updated changelog * fixing a bad changelog merge * oh that style was important * Update src/components/datagrid/body/data_grid_row_manager.ts Co-authored-by: Constance <constancecchen@users.noreply.github.com> * Update src/components/datagrid/data_grid_types.ts Co-authored-by: Constance <constancecchen@users.noreply.github.com> * Ensure unique ID in combobox with prepended or appended labels (#5229) * Making an explicit check for ID prop in EUI Combobox * Checking for a user-passed ID prop * If no user-passed ID, we'll use the `rootId()` class method to set one * Prepend and append props need an ID to relate label to the input * Adding example for Combobox with prepended label * Confirmed the label `for` attribute is referencing input `id` * Added styles to round the edges of the child `<div>` when we prepend a label * Bumped package.json * Adding CHANGELOG entry and bumping version * Update combo_box.tsx * Updating styles to match other forms with prepend and apppend. * Finishing Combobox CSS and test updates * Simplified the Amsterdam overrides for prepended / appended labels * Added conditional classes to `combo_box.tsx` * Added unit tests for user passed ID, prepended and appended classes * Reordering props in child component. * Final adjustment to ID check in ComboBox * Final updates to styling and documentation * Added CSS rules for compressed, prepend and append border radius * Updated the Display Toggles to include append * Removed a duplicated instruction paragraph * Simplified the logic for a unique ID in combobox.ts * Adding Combobox unique ID to master changelog * Updated Form Control Layout SCSS * The borders were still rounded with prepend and append enabled * Added two more rules to make borders square for full height and compressed * Move to _combo_box.scss Co-authored-by: cchaos <caroline.horn@elastic.co> * update i18ntokens * 40.0.0 * Updated documentation. * Add combined Jest+Cypress code coverage reports (#5262) * Install Cypress code coverage dependencies * Set up required babel+istanbul config * Set up Cypress code coverage config @see https://github.com/cypress-io/code-coverage#instrument-unit-tests * Add script for combining cypress & jest code coverage -into a single json & html report * Misc cleanup/ignores - Ignore new `.nyc_output` dir generated by cypress/istanbul - Improve Jest config collectCoverageFrom !ignores: - DRY out component+services folder patterns with a {,} glob - Ignore new Cypress .spec files and .testenv files for jest coverage - eslintignore the cypress folder, since they're all .js anyway and it has a decent amount of idiosyncracies + cleanup unused eslint-disable + cleanup cypress comment boilerplate + remove unused paths from webpack config * Add wiki documentation * [PR feedback] Move nyc config to own file * [PR feedback] Switch combine coverage script to .sh * [EuiMarkdownEditor] Add `remark-breaks` and line break plugin (#5272) * Add remark-breaks and line break plugin to MarkdownEditor * Update changelog * [Docs] Separated out Borders to its own page & [EuiTableRowCell] fixes (#5283) * Moved Border tokens to its own page * Figured out a way to get subsections in the sidenav working * Added the new `_values` under `customizing/` * [EuiTableRowCell] Actually support `valign` property manually * [EuiTable*] Fixing some props and mobile options * Slightly better responsive table styles * Fix CL from #5272 * [Docs] Update EuiDatePicker types (#5318) * omit unsupported types * resolve console errors * CL * [Cypress] Add flakey test retries + harden intermittent context menu failures (#5317) * Add Cypress failed test retries to CI run mode * Increase cy.wait on flaky context menu tests (just in case) * More hardening * [Docs] Fix Colors guidelines (#5316) * use useJsonVars * use variable * Update dependency @elastic/charts to ^38.1.0 (#5321) * PR feedback * Added a cypress test for datagrid row rendering * Revert changes(?) to docs/ * revert changelog reformats * re-ignore a couple datagrid example pages when running a11y tests * Alternate way to know if the datagrid cells have rendered * PR feedback * changelog Co-authored-by: Constance <constancecchen@users.noreply.github.com> Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: ian moersen <ian@union.io> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

1Copenut added 4 commits September 16, 2021 15:18

Making an explicit check for ID prop in EUI Combobox

7c66cc2

* Checking for a user-passed ID prop * If no user-passed ID, we'll use the `rootId()` class method to set one * Prepend and append props need an ID to relate label to the input

Adding example for Combobox with prepended label

73378d9

* Confirmed the label `for` attribute is referencing input `id` * Added styles to round the edges of the child `<div>` when we prepend a label * Bumped package.json

Merge remote-tracking branch 'upstream/master' into trevorp_combobox_id

3ffdd53

Master merge from Elastic EUI upstream

Adding CHANGELOG entry and bumping version

6e93b4a

1Copenut added bug accessibility labels Sep 30, 2021

1Copenut self-assigned this Sep 30, 2021

Update combo_box.tsx

f32728a

cchaos suggested changes Sep 30, 2021

View reviewed changes

1Copenut marked this pull request as ready for review September 30, 2021 20:17

1Copenut requested review from cee-chen, chandlerprall and thompsongl September 30, 2021 20:17

Updating styles to match other forms with prepend and apppend.

974165a

1Copenut requested a review from cchaos September 30, 2021 20:27

Updating CHANGELOG conflict

e70a6b1

cchaos reviewed Sep 30, 2021

View reviewed changes

src/themes/eui-amsterdam/overrides/_form_control_layout.scss Outdated Show resolved Hide resolved

cee-chen reviewed Sep 30, 2021

View reviewed changes

src/components/combo_box/combo_box.tsx Outdated Show resolved Hide resolved

src/components/combo_box/combo_box_input/combo_box_input.tsx Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

1Copenut added 2 commits October 1, 2021 10:26

Finishing Combobox CSS and test updates

948b5c0

* Simplified the Amsterdam overrides for prepended / appended labels * Added conditional classes to `combo_box.tsx` * Added unit tests for user passed ID, prepended and appended classes

Reordering props in child component.

3c096fa

1Copenut requested review from cchaos and cee-chen October 1, 2021 15:32

Final adjustment to ID check in ComboBox

679f0a5

cchaos suggested changes Oct 1, 2021

View reviewed changes

src-docs/src/views/combo_box/combo_box_example.js Show resolved Hide resolved

src/themes/eui-amsterdam/overrides/_form_control_layout.scss Outdated Show resolved Hide resolved

1Copenut added 3 commits October 11, 2021 10:47

Final updates to styling and documentation

5014b77

* Added CSS rules for compressed, prepend and append border radius * Updated the Display Toggles to include append * Removed a duplicated instruction paragraph * Simplified the logic for a unique ID in combobox.ts

Merge remote-tracking branch 'upstream/master' into trevorp_combobox_id

0b3ec95

Adding Combobox unique ID to master changelog

ccd4f7e

1Copenut requested a review from cchaos October 11, 2021 16:39

1Copenut commented Oct 13, 2021

View reviewed changes

Updated Form Control Layout SCSS

4d13fd7

* The borders were still rounded with prepend and append enabled * Added two more rules to make borders square for full height and compressed

1Copenut mentioned this pull request Oct 14, 2021

Replace Inspector's EuiPopover with EuiComboBox elastic/kibana#113566

Merged

9 tasks

Move to _combo_box.scss

0f5082a

cchaos approved these changes Oct 18, 2021

View reviewed changes

thompsongl approved these changes Oct 18, 2021

View reviewed changes

1Copenut added 2 commits October 19, 2021 11:44

Merged from upstream/master

47aea43

Merge branch 'trevorp_combobox_id' of github.com:1Copenut/eui into tr…

953d91e

…evorp_combobox_id

1Copenut merged commit b4e1b65 into elastic:master Oct 19, 2021

1Copenut deleted the trevorp_combobox_id branch October 19, 2021 17:27

thompsongl mentioned this pull request Oct 19, 2021

Upgrade EUI to v40.0.0 elastic/kibana#115639

Merged

Conversation

1Copenut commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Visual Testing Matrix

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Sep 30, 2021

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Oct 1, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Oct 1, 2021

Uh oh!

kibanamachine commented Oct 11, 2021

Uh oh!

1Copenut Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

1Copenut Oct 13, 2021

Choose a reason for hiding this comment

Uh oh!

1Copenut Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 14, 2021

Uh oh!

1Copenut commented Oct 18, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 18, 2021

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

1Copenut commented Sep 30, 2021 •

edited

Loading