Added data-test-subj to EuiFormControlLayout component in super_date_picker file by Vivek-Kamboj · Pull Request #5085 · elastic/eui

Vivek-Kamboj · 2021-08-25T15:35:23Z

Closes #5040

Updates EuiSuperDatePicker to pass a data-test-subj prop.

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for the any docs 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

…picker file

kibanamachine · 2021-08-25T15:35:25Z

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?

cchaos · 2021-08-25T15:59:11Z

src/components/date_picker/super_date_picker/super_date_picker.tsx

            className="euiSuperDatePicker"
            isDisabled={isDisabled}
            prepend={quickSelect}
+            data-test-subj="superDatePickerRange"


Hi @Vivek-Kamboj. Thanks for this addition, but it actually needs to be customizable by the user. We'll need to pass through data-test-subj as a prop on the component.

Then you can also follow these testing guidelines to add a test for this specific prop as part of the PR checklist.

@cchaos, I have customized the data-test-subj and added a test case for that.
Please review.

cchaos · 2021-08-25T19:47:37Z

src/components/date_picker/super_date_picker/super_date_picker.tsx

+  /**
+   * Props passed to tag element for testing
+   */
+  dataTestSubj?: SuperDatePickerDataTestSubj;


Sorry for the confusion usually this is just a string like:

Suggested change

dataTestSubj?: SuperDatePickerDataTestSubj;

'data-test-subj'?: string;

Can you explain your thought process with SuperDatePickerDataTestSubj?

Also, please update the PR summary with your solution and mark the checklist items you were able to perform.

Hi @cchaos,
In the previous comment, you have said that the data-test-subj should be customizable by the user.
And this key is used 2 more times in the file(i.e., for showDates and applyTime). So I have made an object dataTestSubj with interface SuperDatePickerDataTestSubj (declared in src/components/date_picker/types.ts file.)

Now user can pass dataTestSubj object as a prop, and then it is used for the data-test-subj key at different places.

I'd push against this new object-based dataTestSubj structure, as it's one that hasn't been used in EUI before and doesn't match our current patterns.

@Vivek-Kamboj - the main data-test-subj prop that gets passed to the top level <EuiSuperDatePicker /> should be customizable and passed as a string. This usually gets passed to the most applicable DOM element (either a top-level wrapping parent, or the most relevant input, if there it's a form component). Sub-components or child elements such as the Show Dates button typically do not get dynamic/custom data-test-subj strings, instead they get static strings (as exists in the current code). If users want to hook into those, what they would do is something like this:

<EuiSuperDatePicker data-test-subj="mySuperDatePicker" /> cy .find('[data-test-subj="mySuperDatePicker"]') .find(['data-test-subj="superDatePickerShowDatesButton"]') .click();

Hope that makes sense and sheds more light into the implementation we're looking for!

Hello @constancecchen,
I have made changes as per review comments.

Please review.

cee-chen

I have a couple small change requests around tests and docs - once those are addressed, this LGTM. Thanks @Vivek-Kamboj!

cee-chen · 2021-08-30T15:34:55Z

src/components/date_picker/super_date_picker/super_date_picker.test.tsx

+  test('EuiFormControlLayout contain data-test-subj as prop', () => {
+    const dataTestSubj: EuiSuperDatePickerProps['dataTestSubj'] = "mySuperDatePicker";
+
+    const component = render(


If there isn't specific reason to use render() here instead of just shallow(), I'd suggest using shallow for simplicity/performance

I read about shallow() rendering, learned a new way to render. Thank you for sharing @constancecchen

cee-chen · 2021-08-30T15:35:37Z

src/components/date_picker/super_date_picker/super_date_picker.test.tsx

+    const component = render(
+      <EuiSuperDatePicker
+        onTimeChange={noop}
+        dataTestSubj={dataTestSubj}


For simplicity I would suggest just passing it a raw string (like an end-user would) rather than defining a const up above

Suggested change

dataTestSubj={dataTestSubj}

dataTestSubj="mySuperDatePicker"

Okay, I made the changes, please review.

cee-chen · 2021-08-30T15:42:47Z

src/components/date_picker/super_date_picker/super_date_picker.tsx

+  /**
+   * Props passed to tag element for testing
+   */


I'm looking at our current documentation site and the data-test-subj prop, and it looks like we don't include a specific explanation/comment for the prop across any other components. I think we can probably lose this as well here:

Suggested change

/**

* Props passed to tag element for testing

*/

Yes, the variable name is self-explanatory.
I removed this comment.

cee-chen · 2021-08-30T15:43:09Z

src/components/date_picker/super_date_picker/super_date_picker.tsx

            className="euiSuperDatePicker"
            isDisabled={isDisabled}
            prepend={quickSelect}
+            data-test-subj={dataTestSubj}


💯 I like this choice for the data-test-subj location (the same wrapper as .euiSuperDatePicker) over the parent flex-group a lot. Nice work!

cee-chen · 2021-08-30T15:43:26Z

src/components/date_picker/super_date_picker/super_date_picker.tsx


  renderUpdateButton = () => {
-    if (!this.props.showUpdateButton || this.props.isAutoRefreshOnly) {
+    const { showUpdateButton, isAutoRefreshOnly, isLoading, isDisabled, updateButtonProps } = this.props;


💯 Nice, I like the destructuring refactor here!

cee-chen

Changes look great - thanks a million @Vivek-Kamboj!

@cchaos do we normally push up CHANGELOG.md entries on behalf of community PRs? If so, I can do that here and merge

cee-chen · 2021-08-30T19:11:51Z

Just heard back from Caroline, it sounds like we encourage contributors to write their own changelog if they're comfortable - @Vivek-Kamboj do you mind updating your PR description and adding an entry to CHANGELOG.md with a 1-sentence summary of the changes? Something like

Updated `EuiSuperDatePicker` to pass a `data-test-subj` prop

would be fine!

…ue-5040

Vivek-Kamboj · 2021-08-30T19:33:32Z

Update CHANGELOG.md as per review comment.

CHANGELOG.md

cee-chen · 2021-08-30T19:44:16Z

jenkins test this

cee-chen · 2021-08-30T19:45:58Z

@cchaos This LGTM! Do you mind re-reviewing to lose the change requested PR status? Or can we merge in (after CI) despite that?

cchaos

LGTM, thanks for the changes. I just had one more small change to the test itself so that it reads more like a sentence in the snapshot. You'll need to re-update the snapshot too.

cchaos · 2021-08-30T20:05:33Z

src/components/date_picker/super_date_picker/super_date_picker.test.tsx

    expect(component.find(EuiButton).props()).toMatchObject(updateButtonProps);
  });
+
+  test('EuiFormControlLayout contain data-test-subj as prop', () => {


Suggested change

test('EuiFormControlLayout contain data-test-subj as prop', () => {

test('accepts data-test-subj and passes to EuiFormControlLayout', () => {

I have changed the test description and updated the snapshot.

kibanamachine · 2021-08-30T20:10:55Z

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

cchaos · 2021-08-30T20:36:19Z

Looks like there's a linter/prettier issue as well that needs fixing:

15:52:18 /app/src/components/date_picker/super_date_picker/super_date_picker.tsx
15:52:18   481:12  error  Replace `·showUpdateButton,·isAutoRefreshOnly,·isLoading,·isDisabled,·updateButtonProps` with `⏎······showUpdateButton,⏎······isAutoRefreshOnly,⏎······isLoading,⏎······isDisabled,⏎······updateButtonProps,⏎···`  prettier/prettier
15:52:18   518:19  error  Insert `,`                                                                                                                                                                                                         prettier/prettier
15:52:18 
15:52:18 ✖ 2 problems (2 errors, 0 warnings)
15:52:18   2 errors and 0 warnings potentially fixable with the `--fix` option.

… Issue-5040

Vivek-Kamboj · 2021-08-31T03:47:30Z

I have fixed the prettier issues. Please review.

cchaos · 2021-08-31T13:31:30Z

Jenkins, test this

kibanamachine · 2021-08-31T13:55:53Z

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

Added data-test-subj to EuiFormControlLayout component in super_date_…

2d1ae5e

…picker file

Vivek-Kamboj mentioned this pull request Aug 25, 2021

[EuiSuperDatePicker] data-test-subj doesn't show up as an attribute in rendered DOM #5040

Closed

cchaos suggested changes Aug 25, 2021

View reviewed changes

Issue elastic#5040, Customize data-test-subj and added test case for it

970e7fc

cchaos reviewed Aug 25, 2021

View reviewed changes

cee-chen linked an issue Aug 26, 2021 that may be closed by this pull request

[EuiSuperDatePicker] data-test-subj doesn't show up as an attribute in rendered DOM #5040

Closed

Issue elastic#5040, Modified data-test-subj prop as per review comment

bbf3abe

cee-chen reviewed Aug 30, 2021

View reviewed changes

Issue elastic#5040, Modified as per review comment

0c1abc5

cee-chen approved these changes Aug 30, 2021

View reviewed changes

Vivek-Kamboj added 2 commits August 31, 2021 00:55

Issue elastic#5040, Updated changelog.md

021fdd2

Merge branch 'master' of https://github.com/Vivek-Kamboj/eui into Iss…

c73690a

…ue-5040

cee-chen reviewed Aug 30, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

8d67a95

cee-chen requested a review from cchaos August 30, 2021 19:45

cchaos changed the title ~~Issue-#5040 || Added data-test-subj to EuiFormControlLayout component in super_date_picker file~~ Added data-test-subj to EuiFormControlLayout component in super_date_picker file Aug 30, 2021

cchaos approved these changes Aug 30, 2021

View reviewed changes

Vivek-Kamboj added 2 commits August 31, 2021 09:13

Issue elastic#5040, fixed prettier issues

b6d048e

Merge branch 'Issue-5040' of https://github.com/Vivek-Kamboj/eui into…

6fd0e2d

… Issue-5040

cchaos enabled auto-merge (squash) August 31, 2021 13:43

cchaos merged commit a7e77f4 into elastic:master Aug 31, 2021

thompsongl mentioned this pull request Sep 2, 2021

Upgrade EUI to v37.6.0 elastic/kibana#110998

Merged

cchaos mentioned this pull request Nov 16, 2021

[EuiSuperDatePicker] Fixes for width and auto refresh #5383

Merged

10 tasks

	dataTestSubj?: SuperDatePickerDataTestSubj;
	'data-test-subj'?: string;

	test('EuiFormControlLayout contain data-test-subj as prop', () => {
	test('accepts data-test-subj and passes to EuiFormControlLayout', () => {

Conversation

Vivek-Kamboj commented Aug 25, 2021 • edited by cchaos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #5040

Checklist

Uh oh!

kibanamachine commented Aug 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 30, 2021

Uh oh!

Vivek-Kamboj commented Aug 30, 2021

Uh oh!

Uh oh!

cee-chen commented Aug 30, 2021

Uh oh!

cee-chen commented Aug 30, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Aug 30, 2021

Uh oh!

cchaos commented Aug 30, 2021

Uh oh!

Vivek-Kamboj commented Aug 31, 2021

Uh oh!

cchaos commented Aug 31, 2021

Uh oh!

kibanamachine commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Vivek-Kamboj commented Aug 25, 2021 •

edited by cchaos

Loading

cee-chen Aug 26, 2021 •

edited

Loading