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

Fix datagrid issue in Discover for Firefox #90906

Merged
merged 3 commits into from
Feb 11, 2021
Merged

Fix datagrid issue in Discover for Firefox #90906

merged 3 commits into from
Feb 11, 2021

Conversation

snide
Copy link
Contributor

@snide snide commented Feb 10, 2021

Summary

The data grid in Discover when no fields are selected would run into an issue because of a word wrap issue the way the team was using EuiDescriptionList. A small overwrite is all that was needed.

Thanks for the spot @ryankeairns. @chandlerprall you can safely breathe as this wasn't a problem with EUI.

After

image

Before

image

Checklist

Delete any items that are not applicable to this PR.

@snide snide requested review from ryankeairns and a team February 10, 2021 03:32
@snide snide requested a review from a team as a code owner February 10, 2021 03:32
@snide snide added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 10, 2021
snide
snide commented Feb 10, 2021
View reviewed changes
src/plugins/discover/public/application/components/discover_grid/discover_grid.scss
Comment on lines +12 to +18
.euiDataGridRowCell.euiDataGridRowCell--firstColumn {
border-left: none;
}

.euiDataGridRowCell.euiDataGridRowCell--lastColumn {
border-right: none;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some hacky stuff to adapt to the way data grid now handles borders. Fixes a visual problem where they had double borders.

ryankeairns
ryankeairns approved these changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Your Firefox friends are gateful! Thank you

Unrelated nit: upon load, the header has a gap that goes away upon scrolling the grid.

Screen Shot 2021-02-09 at 9 50 33 PM

@snide
Copy link
Contributor Author

snide commented Feb 10, 2021

@elasticmachine merge upstream

kertal
kertal reviewed Feb 10, 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.

Yes, I said, yes, but adapting the number of docs seems to break several functional test, I intend to focus on that stuff tomorrow, so it might make sense to fix it in a separate PR, since just adopting the numbers is not enough here, I really wonder why in one of this tests there are 250 rows returned, I'd expect less due to virtualization. But what would be nice to fix, since Firefox stuff is improved, is the overlapping of header row with the first data row

@snide
Copy link
Contributor Author

snide commented Feb 10, 2021 •
edited
Loading

@kertal OK. I'll remove that commit, and merge on green with the css changes.

@kertal
Copy link
Member

kertal commented Feb 11, 2021

@elasticmachine merge upstream

@kertal kertal self-requested a review February 11, 2021 08:46
kertal
kertal approved these changes Feb 11, 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 locally, main Firefox issue fixed. Tried to fix the header position, but it's a kind of mystery, so it should be solved in a different PR.

@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 420.8KB 422.0KB +1.2KB

History

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

@snide snide merged commit facdd55 into elastic:master Feb 11, 2021
@snide snide deleted the dg/inline_fix branch February 11, 2021 15:47
@snide snide mentioned this pull request Feb 11, 2021
Merged
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 15, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@flash1293
Copy link
Contributor

@snide Should we merge the backport?

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 17, 2021
snide added a commit that referenced this pull request Feb 17, 2021
@snide @kibanamachine
Fix datagrid issue in Discover for Firefox (#90906) (#91253)
adc62c3 
* Fix datagrid issue in Discover for Firefox
* small visual cleanup while im in here

Co-authored-by: Kibana Machine <[email protected]>
@kertal kertal mentioned this pull request Feb 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@snide @kertal @kibanamachine @flash1293 @ryankeairns