Skip to content

EuiTablePagination: Adds data-test-subj to the EuiContextMenuItem#2778

Merged
chandlerprall merged 3 commits intoelastic:masterfrom
ffknob:testing-EuiTablePagination-data-test-subj
Jan 21, 2020
Merged

EuiTablePagination: Adds data-test-subj to the EuiContextMenuItem#2778
chandlerprall merged 3 commits intoelastic:masterfrom
ffknob:testing-EuiTablePagination-data-test-subj

Conversation

@ffknob
Copy link
Copy Markdown
Contributor

@ffknob ffknob commented Jan 20, 2020 •
edited
Loading

Summary

Closes #2762

Added data-test-subj to the EuiContextMenuItem in EuiTablePagination

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jan 20, 2020

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?

This was referenced Jan 20, 2020
Closed
Open
@chandlerprall chandlerprall self-assigned this Jan 21, 2020
@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jan 21, 2020

Thanks for submitting this!

We'd like data-test-subj to specify the number of rows represented by each option, and in a way the developer doesn't have to explicitly provide the prop as the only data here is an array of numbers and there isn't a great way to give individual test props. For our components which are more internal (it's unlikely someone would use EuiTablePagination directly), it's common to hard-code the data-test-subj instead of accepting it as a prop.

The attribute here would be something like

data-test-subj={`tablePagination-${itemsPerPageOption}-rows`}

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jan 21, 2020

Needs an entry to CHANGELOG.md, but otherwise looks good.

@chandlerprall
Copy link
Copy Markdown
Contributor

chandlerprall commented Jan 21, 2020

jenkins test this

chandlerprall
chandlerprall approved these changes Jan 21, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Changes LGTM; pulled and tested locally

@chandlerprall chandlerprall merged commit c9dcfb3 into elastic:master Jan 21, 2020
andreadelrio pushed a commit to andreadelrio/eui that referenced this pull request Jan 22, 2020
@ffknob @andreadelrio
EuiTablePagination: Adds data-test-subj to the EuiContextMenuItem (el…
1cda9af 
…astic#2778)

* Adds data-test-subj to the EuiContextMenuItem component rendered in EuiTablePagination

* Changes how the data-test-subj value is set

* Adds CHANGELOG line
@thompsongl thompsongl mentioned this pull request Jan 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chandlerprall chandlerprall chandlerprall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chandlerprall chandlerprall

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EuiTablePagination should apply data-test-subj to options

3 participants

@ffknob @kibanamachine @chandlerprall