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

[Doc-Table] Import styles into JS where possible #65329

Merged
merged 1 commit into from
May 6, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented May 5, 2020

Follow up to #63473

It was noticed that table vis' in dashboard were no longer getting the styles they needed to render and scroll correctly.

Screen Shot 2020-05-05 at 12 45 25 PM

This PR fixes that by importing the SASS files to the associated JS component files. It does it for mostly all of the Discover components except for any that are still angular and the top-level "hacks".

Before

Screen Shot 2020-05-05 at 14 12 55 PM

After

Screen Shot 2020-05-05 at 14 14 33 PM

Screen Shot 2020-05-05 at 14 11 41 PM

Checklist

@cchaos cchaos added Feature:Data Table Data table visualization feature Feature:Discover Discover Application release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.8.0 labels May 5, 2020
@cchaos cchaos requested review from flash1293, sulemanof and a team May 5, 2020 18:25
@cchaos cchaos requested a review from a team as a code owner May 5, 2020 18:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in chrome and overflow problem is fixed even when directly loading dashboard and not going to any other page. Discover still looks fine as well.

I think the problem can be solved with less changes but I definitely like this pattern of putting the styles directly next to the component which is using them - makes it much more future proof!

LGTM 👍

@flash1293
Copy link
Contributor

A quick note: 7.8 got branched off while this PR was open, it should be backported in 7.8 and 7.x .

@flash1293
Copy link
Contributor

@cchaos You added the fix label, was this bug ever released? If not I think we can skip this one for the changelog

@sulemanof
Copy link
Contributor

A quick note: 7.8 got branched off while this PR was open, it should be backported in 7.8 and 7.x .

7.8 doesn't have those changes, should be backported only to 7.x

@flash1293
Copy link
Contributor

Ah oops, of course. Thanks for correcting me @sulemanof !

@cchaos
Copy link
Contributor Author

cchaos commented May 6, 2020

Thanks! I'll remove the label and only backport to 7.x (change 7.8 to 7.9)

@cchaos cchaos added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix v7.8.0 labels May 6, 2020
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM.

@cchaos cchaos merged commit e5abec1 into elastic:master May 6, 2020
cchaos added a commit that referenced this pull request May 6, 2020
@cchaos cchaos deleted the fix/doc-table-styles branch October 1, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Table Data table visualization feature Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants