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.

Sign up for GitHub

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

Jump to bottom

[EuiBasicTable] Fix generated IDs regenerating on rerender #5196

Merged
merged 2 commits into from
Sep 20, 2021
Merged

[EuiBasicTable] Fix generated IDs regenerating on rerender #5196

merged 2 commits into from
Sep 20, 2021

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 16, 2021 •
edited
Loading

Summary

closes #3111

Related parent PR:

Checklist

QA screencaps below in code comments.

Changelog will be a single entry in #5195.

cee-chen
cee-chen commented Sep 16, 2021
View reviewed changes
Copy link
Contributor Author

@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.

QA

src/components/basic_table/basic_table.tsx
@@ -762,7 +763,7 @@ export class EuiBasicTable<T = any> extends Component<
<EuiI18n token="euiBasicTable.selectAllRows" default="Select all rows">
{(selectAllRows: string) => (
<EuiCheckbox
id={`_selection_column-checkbox_${htmlIdGenerator()()}`}
id={this.selectAllCheckboxId}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before:
select_all_checkbox_before

After:
select_all_checkbox_after

src/components/basic_table/default_item_action.tsx
@@ -57,12 +57,12 @@ export const DefaultItemAction = <T extends {}>({
let button;
const actionContent =
typeof action.name === 'function' ? action.name(item) : action.name;
const ariaLabelId = useGeneratedHtmlId();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before:
basic_table_action_before

After:
basic_table_action_after

@cee-chen cee-chen mentioned this pull request Sep 16, 2021
Merged
1 task
@kibanamachine
Copy link

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

@cee-chen cee-chen mentioned this pull request Sep 16, 2021
Merged
thompsongl
thompsongl approved these changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍
I think it's worth adding a bugfix changelog entry. Rerender behavior is changing so it seems like more than just an "internals" update. If you want to add a single entry that references all the related PRs, I'm good with that.

@cee-chen
Copy link
Contributor Author

cee-chen commented Sep 20, 2021 •
edited
Loading

Sweet, thanks Greg! I'll likely end up using #5195 as the main changelog PR (since it links out to all the other related PRs) and merge this one as-is!

@cee-chen cee-chen merged commit 571ebc4 into elastic:master Sep 20, 2021
@cee-chen cee-chen deleted the use-generated-html-id-basic-table branch September 20, 2021 16:02
@breehall breehall mentioned this pull request Oct 1, 2021
Merged
@cee-chen cee-chen mentioned this pull request Oct 4, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

Assignees
No one assigned
Labels
skip-changelog tech debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiBasicTable] "Select all" checkbox regenerates id on each render
3 participants
@cee-chen @kibanamachine @thompsongl