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

[EuiTableHeader] Add prop to remove <tr> #4465

Merged
merged 17 commits into from
Feb 3, 2021

Conversation

K-Kumar-01
Copy link
Contributor

@K-Kumar-01 K-Kumar-01 commented Jan 31, 2021

Summary

Issue number #4183
Added the ability to remove the <tr/> tag.

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

K-Kumar-01 and others added 12 commits December 15, 2020 16:01
Issue number 4239
Ability to unregister the tooltip plugin added
Updated the editor example doc to show how to use it
Added the ability to unregsiter inbuilt UI plugins
Addresses issue 4239

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
Default UI Plugins initialized
Warning added on injecting tooltip plugin

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
Issue number 4183

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
@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?

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.

Thanks for tackling this one @K-Kumar-01 . There are still some more necessary things to complete this PR, I'll update the checklist in PR summary by crossing out the ones that aren't necessary. But at top level, we'll at least need a new test for this prop and a changelog entry.

src/components/table/table_header.tsx Outdated Show resolved Hide resolved
src/components/table/table_header.tsx Outdated Show resolved Hide resolved
@cchaos cchaos changed the title Eui table header 4183 [EuiTableHeader] Add prop to remove <tr> Feb 1, 2021
@cchaos
Copy link
Contributor

cchaos commented Feb 1, 2021

As a side-note, when creating new PR's it's always best to start a new branch from the latest master branch. That way, your commit log will only include commits directly related to this PR.

@K-Kumar-01
Copy link
Contributor Author

As a side-note, when creating new PR's it's always best to start a new branch from the latest master branch. That way, your commit log will only include commits directly related to this PR.

I will keep in mind the next time I make a PR.:)

@K-Kumar-01
Copy link
Contributor Author

K-Kumar-01 commented Feb 2, 2021

@cchaos
I have made the required changes in the file.
However, I am facing issues with writing tests as I have not written any tests earlier.

I did some test and finally, I have 2 results:
On using shallow the test passes
Screenshot from 2021-02-02 12-25-02

On using render, the test fails with the error as shown
Screenshot from 2021-02-02 12-26-17
I am not able to understand how to rectify this one. In components/basic_table/collapsed_items_actions.tsx also has a similar approach.

I do not know much about tests so I do not know if we can use shallow or not.

@cchaos
Copy link
Contributor

cchaos commented Feb 2, 2021

@K-Kumar-01 Can you commit what you currently have? I'm guessing it has to do with the actual render() of the component without the <tr> and not a fault of the test. (The test doing the right thing 😉 )

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>
@K-Kumar-01
Copy link
Contributor Author

K-Kumar-01 commented Feb 3, 2021

@cchaos I have committed the recent changes. Let me know if there are any changes required.

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.

So it looks like the way our testing suite is managed, is there is some level of DOM validation happening when creating snapshots. So that meant, when you were providing a simple string as the child of a <tr>, it said "nope 😆 " and appended the invalid child outside of the <tr> instead. So I fixed up the tests by passing the valid DOM structure and it correctly renders the snapshots.

I also updated the name of the prop.

👍 LGTM

@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

Jenkins, test this

1 similar comment
@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

Jenkins, test this

@K-Kumar-01
Copy link
Contributor Author

So it looks like the way our testing suite is managed, is there is some level of DOM validation happening when creating snapshots. So that meant, when you were providing a simple string as the child of a <tr>, it said "nope laughing " and appended the invalid child outside of the <tr> instead. So I fixed up the tests by passing the valid DOM structure and it correctly renders the snapshots.

I also updated the name of the prop.

+1 LGTM

Thanks a lot for the tests. I will try my best to write learn writing tests 😃 and also ensure to pass Valid DOM structure 😬

@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

Oh no worries. It caught it me off guard too and had to play around with the children I passed to see exactly what was happening. First time I've seen this, so good find! 😜

Jenkins, test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

Jenkins, test this

@kibanamachine
Copy link

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

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.

4 participants