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

Use generated IDs in EuiButtonGroup Buttons #4657

Merged

Conversation

danedavid
Copy link
Contributor

Fixes #4645

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Mar 22, 2021

💚 CLA has been signed

@chandlerprall chandlerprall added skip-changelog documentation Issues or PRs that only affect documentation - will not need changelog entries labels Mar 22, 2021
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Thanks for making this change! After testing the modified example, the other options' ids would need to be made unique as well:

stylingids

Which demonstrates how easy it is to build a UI with accidentally overlapping object IDs. It's trivial for an application to build separate panels on the same page and accidentally use the same IDs, or maybe they're provided by a user and the application has no control. Because the id concept in EuiButtonGroup is to track an option just within itself, I think we should avoid writing the provided id to the DOM and instead generate a random one for the component to use.

  • undo the changes you made to styling.js (thank you again for picking this up and making the change requested in the issue)
  • In button_group_button.tsx use the htmlIdGenerator utility to generate & store a random ID, see https://github.com/elastic/eui/blob/master/src-docs/src/views/form_layouts/form_rows.js#L21 for an example
  • use the generated ID on the existing htmlFor: newId,, id={newId},, and id, (-> id: newId,) lines
  • while we didn't intend to support this directly, it's possible unit or functional tests rely on that id to be present, so let's also add a data-test-subj: id to the singleInput and the else's elementProps, so tests can still find their target elements

This will separate the intended use of the ids - separately tracking options - from their DOM implementation and should fix the styling example.

Don't hesitate to ask for clarification if any of that doesn't make sense.

@danedavid
Copy link
Contributor Author

@chandlerprall Made the changes. Had to update snapshots as the snapshot contained ids

@danedavid danedavid changed the title Change ids for EuiButtonGroup in EuiDataGrid example in docs Use generated IDs in EuiButtonGroup Buttons Mar 24, 2021
@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Great, thank you for taking this & making additional changes! I also added an entry to CHANGELOG.md as it is no longer a documentation-only change.

Tested the datagrid styling example locally and all button groups now act as expected. Verified the ids in the DOM are generated at mount and not updated.

@chandlerprall chandlerprall merged commit 4a2e8e1 into elastic:master Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation Issues or PRs that only affect documentation - will not need changelog entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Styling example uses invalid DOM
3 participants