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

[EuiDescriptionList] titleProps and descriptionProps extends CommonProps #5166

Merged
merged 9 commits into from
Sep 20, 2021

Conversation

MukulKolpe
Copy link
Contributor

@MukulKolpe MukulKolpe commented Sep 10, 2021

Fixes #5148

EuiDescriptionListProps is extending CommonProps.

Checklist

~- [ ] Check against all themes for compatibility in both light and dark modes
~- [ ] Checked in mobile
~- [ ] Checked in Chrome, Safari, Edge, and Firefox

~- [ ] Added documentation
~- [ ] Checked Code Sandbox works for any docs examples

~- [ ] Checked for breaking changes and labeled appropriately
~- [ ] Checked for accessibility including keyboard-only and screen reader modes

  • A changelog entry exists and is marked appropriately

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 10, 2021

💚 CLA has been signed

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

@thompsongl
Copy link
Contributor

Hey thanks, @MukulKolpe!

Can you please sign the CLA if you haven't already?

@MukulKolpe
Copy link
Contributor Author

I signed it actually

@thompsongl thompsongl self-requested a review September 10, 2021 15:41
@thompsongl
Copy link
Contributor

Also, @MukulKolpe, be sure to check off or cross out all checklist items. For instance, this PR will require a changelog entry.

@MukulKolpe MukulKolpe closed this Sep 10, 2021
@MukulKolpe MukulKolpe reopened this Sep 10, 2021
@MukulKolpe MukulKolpe changed the title EuiDescriptionListProps extends CommonProps titleProps and descriptionProps extends CommonProps Sep 11, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
src/components/description_list/description_list.tsx Outdated Show resolved Hide resolved
src/components/description_list/description_list.tsx Outdated Show resolved Hide resolved
@cchaos cchaos changed the title titleProps and descriptionProps extends CommonProps [EuiDescriptionList] titleProps and descriptionProps extends CommonProps Sep 13, 2021
@cchaos
Copy link
Contributor

cchaos commented Sep 13, 2021

I've also unchecked the PR checklist item about jest tests. We should add a specific test for passing the requiredProps to each of these props in https://github.com/elastic/eui/blob/588df390546fc7439ae1cdefcd09cfe192117002/src/components/description_list/description_list.test.tsx.

For example, in that file, if you move the const listItems = [...] to outside of the testing block to reuse, then you can add something like:

  describe('titleProps', () => {
      test('is rendered', () => {
        const component = render(<EuiDescriptionList listItems={listItems} titleProps={requiredProps} />);

        expect(component).toMatchSnapshot();
      });
    });

@thompsongl
Copy link
Contributor

jenkins test this

@MukulKolpe
Copy link
Contributor Author

@cchaos
Copy link
Contributor

cchaos commented Sep 14, 2021

That was just a failed run. And yes, that is the file to add the tests too. After adding them, be sure to run yarn test-unit -u to update/create the snapshots. Once you'd pushed those changes we can re-run CI.

@cchaos
Copy link
Contributor

cchaos commented Sep 16, 2021

Jenkins, test this

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

jenkins test this

@thompsongl thompsongl self-requested a review September 16, 2021 19:51
@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 LGTM! Thanks for the PR @MukulKolpe

@MukulKolpe
Copy link
Contributor Author

Thank you @thompsongl and @cchaos for helping me with this issue!!

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks, @MukulKolpe!

@thompsongl thompsongl merged commit 1f4c43a into elastic:master Sep 20, 2021
@MukulKolpe MukulKolpe deleted the EuiDescriptionList branch May 13, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDescriptionList] Allow data-test-subj
4 participants