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

[Discover] Fix Data grid columns movement #115279

Merged
merged 14 commits into from
Nov 3, 2021
Merged

[Discover] Fix Data grid columns movement #115279

merged 14 commits into from
Nov 3, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Oct 15, 2021 •
edited
Loading

Summary

Fixes #115075

This PR fixes time filed disappiaring when it was selected and hideTimeColumn was set to true for data grid

Test notes

  • Set hideTimeColumn to true and doc_table:legacy to false
  • Open discover
  • Select some fields and then time field
  • Move time field to the first place
  • Time field should not be dissapiared

@dimaanj dimaanj added Feature:Discover Discover Application release_note:fix v8.0.0 v7.16.0 Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Oct 15, 2021
@dimaanj dimaanj self-assigned this Oct 15, 2021
@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 16, 2021

@elasticmachine merge upstream

@dimaanj dimaanj marked this pull request as ready for review October 18, 2021 16:00
@dimaanj dimaanj requested a review from a team as a code owner October 18, 2021 16:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 18, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 328.9KB 328.8KB -61.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dmitriynj

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 21, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 21, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 24, 2021

@elasticmachine merge upstream

majagrubic
majagrubic reviewed Oct 25, 2021
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Code looks good, will test it and then leave further comments

majagrubic
majagrubic suggested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

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

With this change, Document column seems to be completely empty:
Screenshot 2021-10-26 at 10 38 06

This persists even after refresh, and after changing the date selection.

I checked on master, and that doesn't seem to be the case, so I believe it was caused by this change.

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 28, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 28, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 28, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Nov 1, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Nov 2, 2021

@elasticmachine merge upstream

kertal
kertal approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested a-la-carte, bug is fixed 👍
@majagrubic could you have another look, many thx

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 303.5KB 303.6KB +107.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dmitriynj

majagrubic
majagrubic approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Had a quick pass through this, since @kertal already approved it.
LGTM

@dimaanj dimaanj added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 3, 2021
@dimaanj dimaanj merged commit 6328a9f into elastic:main Nov 3, 2021
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v8.1.0

If any of these should not be on your pull request, please manually remove them.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 3, 2021
@dimaanj @kibanamachine
[Discover] Fix Data grid columns movement (elastic#115279)
e8285a6 
* [Discover] fix data grid columns movement

* [Discover] apply suggestion

* [Discover] fix linting

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Nov 3, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 3, 2021
@dimaanj @kibanamachine
[Discover] Fix Data grid columns movement (elastic#115279)
02bf0a7 
* [Discover] fix data grid columns movement

* [Discover] apply suggestion

* [Discover] fix linting

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Nov 3, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0
✅ 7.16

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 3, 2021
@kibanamachine @dimaanj
[Discover] Fix Data grid columns movement (#115279) (#117265)
d0194b7 
* [Discover] fix data grid columns movement

* [Discover] apply suggestion

* [Discover] fix linting

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Nov 3, 2021
@kibanamachine @dimaanj
[Discover] Fix Data grid columns movement (#115279) (#117266)
750b30e 
* [Discover] fix data grid columns movement

* [Discover] apply suggestion

* [Discover] fix linting

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@majagrubic majagrubic majagrubic approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees

@dimaanj dimaanj

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v7.16.0 v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timestamp Field Disappears in Discover When Moving Columns
6 participants
@dimaanj @elasticmachine @kibanamachine @kertal @timroes @majagrubic