Visualize LESS to SASS by cchaos · Pull Request #22679 · elastic/kibana

cchaos · 2018-09-04T18:26:10Z

This PR removes the LESS files inside /src/core_plugins/kibana/public/visualize and replaces them with Sass.

Process taken

The /src/core_plugins/kibana/public/visualize folder now includes a manifest _index.scss which is imported by src/core_plugins/kibana/public/index.scss.
- Since styling_constants.scss is imported in the main public/index.scss file, it has access to all the functions, mixins and variables from EUI.
The SASS files are broken down into component folders.
- The sass files now live with separated _index.scss and _component_names.scss files next to the components or views they live next to.
- The new sass files use EUI variables whenever possible. The most important being color and sizing variables.
- The selectors were all changed to match EUI's BEM formatting. This means the html/js templating was touched as well.
- Additionally, a three-letter prefix vis was added to all selectors to namespace them and avoid conflicts.
Most of the selectors found in /src/core_plugins/kibana/public/visualize actually had their html counterparts in /src/ui/public/vis so they were moved there.
- In the interest of not making this PR too huge, I've intentionally left the names of the selectors in src/ui/public/vis/editors/default/_agg.scss alone, to be updated when the rest of the folder does.
ML was importing some of these LESS files directly from visualize, though they seem to only be using the wizard elements. I've gone ahead and created a manifest with the visualize/_index file to start. (cc @snide)

Enhancements

1. Fixed the responsive layout to actually show the visualization on small screens

By changing the flex-direction, the chart now shows below the editing panel (sidebar).

2. Updated the chart type images to use the newly created EUI icons

I only saw to places where these were displayed:
a. Chart type selector in the wizard
b. Listing of all saved charts

Let me know if there are other known places, and I'll be sure to update the <icon/> implementation.

Quick caveat
It uses the <icon /> angular implementation for EuiIcon, but I didn't remove the ng-class for backwards compatibility for plugins. This means, however, that the type and ng-class can be supplied for non-EuiIcon allowed strings and therefore, the old way will now throw a console warning about EuiIcon not being supplied a valid type.

My thoughts are that this is ok, and possibly a very rare case.

Questions

I can't find a plain <visualize /> element anywhere, though there are CSS selectors for it. Is this an outdated selector?
Same with nesting-indicator which is also targeted via tests, but can't find where/how/if it's actually implemented

@elastic/kibana-visualizations Please add a team member to do a review.

Dev Docs

Visualization icons

The icon property when registering a visualization type that accepts a class name (e.g. to set a font awesome icon) has been renamed to legacyIcon and will be removed in 7.0.0.

The icon property now expects a valid EUI icon type. Alternatively to using an EUI icon, you can use the image property instead, which will be set to the src attribute of an img. Thus you can also use the image property to set any image as a data URI (e.g. by importing it via Webpack).

- Moves styles closer to their actual components in `public/vis/…` (agg stuff did not get naming updates) - Also fixes responsive layout to actually show the visualization - Updated to `<icon/>` EUI usage where possible

trevan · 2018-09-04T18:44:50Z

@cchaos, about your caveat. Are you saying that all custom visualization plugins will now throw a console warning?

cchaos · 2018-09-04T19:07:54Z

@trevan Only if you are supplying the icon via a css class. If you supply an image, then nothing will change. So for instance, if a custom visualization is using a Font Awesome icon and passes fa-icon as the icon value, then you will get an error. In which case, you can either pass a valid EuiIcon icon type, or create and pass an image instead.

elasticmachine · 2018-09-04T20:08:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2018-09-04T20:28:42Z

FYI: I found an issue with reporting, I am looking into it.

cchaos · 2018-09-04T22:03:36Z

🤞 That last commit should fix reporting and x-pack tests

elasticmachine · 2018-09-05T00:42:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

trevan · 2018-09-05T03:07:34Z

@cchaos, I just went through all of the "known plugins" and the majority of them use the "fa-icon". So it looks like this error will be very common for users that have custom plugins.

timroes · 2018-09-05T08:28:47Z

cc @walterra I also tagged you as a reviewer, since it touches some ML code

timroes · 2018-09-05T08:38:11Z

src/core_plugins/kibana/public/visualize/editor/_editor.scss

+  // SASSTODO: Is this even an element anymore, or should it be .visualize?
+  /* 1. Without setting this to 0 you will run into a bug where the filter bar modal is hidden under
+a tilemap in an iframe: https://github.com/elastic/kibana/issues/16457 */
+  > visualize {


As you mentioned above, this should actually be .visualize and got broken in some of the Angular refactoring. I will already fix this on master so we can pull the fix in 6.4.[1|2].

timroes · 2018-09-05T09:04:59Z

I would suggest a different solution for the old icon classes. Let's introduce a legacyIcon key. In case no image and no icon is specified, we render the legacyIcon as an ng-class of a plain span element (as it was done beforehand). In case icon is specified, we instead will just render it using <icon> and also not setting it via ng-class so that you HAVE to switch to legacyIcon in case you want to use a Font Awesome icon here. We can then remove that legacyIcon key with 7.0. I think trying to use both ways over the same HTML element, might just cause us more trouble. Also renaming the icon key to legacyIcon in a plugin when increasing the version number is not too much work to do, and it makes it clear, that this key will vanish over time, and instead be replaced by only EUI icon or using any custom image.

Edit: I opened a PR against your repo to introduce that setting.

# Conflicts: # src/core_plugins/kibana/public/visualize/editor/styles/_editor.less

Introduce legacyIcon

elasticmachine · 2018-09-05T16:36:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-06T15:41:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

snide

I did a code run through of this on Tuesday night and everything looks pretty good to me. I haven't had time to do a full browser test, but as long as Viz has that covered this looks good.

snide · 2018-09-06T16:27:55Z

src/core_plugins/kibana/public/visualize/wizard/_wizard.scss

+/**
+ * 1. Hack in some padding for these panels.
+ */
+.visWizard__list--paginated--selectable,


would this just be --paginatedSelectable?

I did it like this because they wording makes them seem like separate modifiers. It's 'paginate' and it's 'selectable'.

cchaos · 2018-09-06T18:34:14Z

@timroes Any thoughts on that second question about the nesting-indicator?

# Conflicts: # src/core_plugins/input_control_vis/public/register_vis.js

elasticmachine · 2018-09-11T23:47:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

chrisdavies · 2018-09-12T12:21:27Z

It looks like this PR is regularly hitting what was otherwise a rare flaky test. Is this by any chance adding (more) animation to the display of the report printing pane? That's the source of the bug. Nathan's getting rid of the report printing pane altogether and instead going with a flyout, so the bug should disappear. @cchaos if you want, I can Zoom today, and we can fix this failing test (by introducing a delay to our test), and then we can make a note to rip the hacky fix out once Nathan's PR is merged.

cchaos · 2018-09-12T14:34:33Z

Thanks @chrisdavies for keeping up with this. I did not introduce more or longer animations. I'd love your help if you have time.

cchaos · 2018-09-12T19:12:29Z

Looking at the difference between the baseline, master's session, and my local session pdf's, we determined that the baseline is currently wrong and that I altered the styles just slightly enough (probably some padding pixel value somewhere) that my version was just passed the allowed variance threshold while master's didn't.

So this PR now also updates the baseline PDF for the "Visualize" reporting test.

elasticmachine · 2018-09-12T20:42:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos · 2018-09-12T20:47:32Z

Tests are passing!!

@elastic/kibana-visualizations, can I get a review, please :)?

walterra

@cchaos please see the comment in the code, noticed a class didn't pick up any styles.

walterra · 2018-09-07T08:27:25Z

x-pack/plugins/ml/public/jobs/new_job/wizard/steps/index_or_search/index_or_search.html

          list-property="attributes.title"
          user-make-url="withIndexPatternUrl"
-          class="wizard-row visualizeWizardPaginatedSelectableList kuiVerticalRhythm"
+          class="wizard-row visWizard__paginatedSelectableList kuiVerticalRhythm"


Checked this in browser and it's not picking up any styles, is this supposed to be visWizard__list--paginated--selectable? (On the other hand, the visWizard__savedObjectFinder further down correctly picks up padding: 8px;)

Ahh yeah, it looks like I renamed it twice and the second time it didn't change in ML. Really good catch, thanks! I'll update that classname.

elasticmachine · 2018-09-13T20:19:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-09-14T09:04:09Z

@cchaos Sorry I was out a couple of days, and haven't seen the 2nd question the first time. I think that is unneeded and dead code. I created a PR to remove it: #23180 so we don't need to mix this into this PR. I try to get this merged quickly, so you can rebase on it, and don't need to worry about any nesting-indicator LESS/SASS anymore.

walterra · 2018-09-14T09:41:47Z

@cchaos Latest ML changes LGTM!

timroes · 2018-09-14T15:08:59Z

@cchaos Merged the PR, you can now merge with master.

cchaos · 2018-09-14T15:17:21Z

Thank you @timroes and @walterra !

# Conflicts: # src/core_plugins/kibana/public/visualize/editor/styles/_editor.less # src/ui/public/styles/variables/for-theme.less

elasticmachine · 2018-09-14T17:12:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-04T15:12:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos added 7 commits August 29, 2018 10:54

added SASS versions

8c741f3

Wizard done

85da4e1

Updating vis type images to use EuiIcons

719fc76

Disabled lab vis done

ed444d1

Moving selectors around and some re-naming

3200a52

- Moves styles closer to their actual components in `public/vis/…` (agg stuff did not get naming updates) - Also fixes responsive layout to actually show the visualization - Updated to `<icon/>` EUI usage where possible

Last bit of cleanup

83c802d

Merge remote-tracking branch '2_upstream/master' into vis-less-to-sass

2b57024

cchaos requested a review from snide September 4, 2018 18:26

cchaos added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.5.0 chore Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. labels Sep 4, 2018

Forgot to check pdf .css for selector name updates

31c91cc

timroes requested a review from walterra September 5, 2018 08:28

timroes self-requested a review September 5, 2018 08:28

timroes reviewed Sep 5, 2018

View reviewed changes

Tim Roes and others added 4 commits September 5, 2018 11:21

Introduce legacyIcon

8c8cc1f

fixing visualize -> .visualize

a10db74

Merge remote-tracking branch '2_upstream/master' into vis-less-to-sass

861b10c

# Conflicts: # src/core_plugins/kibana/public/visualize/editor/styles/_editor.less

Merge pull request #1 from timroes/pr/22679

6ff0d6e

Introduce legacyIcon

snide approved these changes Sep 6, 2018

View reviewed changes

snide mentioned this pull request Sep 6, 2018

[META] Moving Kibana off Less and using EUI global CSS scope #19748

Closed

30 tasks

Merge remote-tracking branch '2_upstream/master' into vis-less-to-sass

e4b5f7f

# Conflicts: # src/core_plugins/input_control_vis/public/register_vis.js

cchaos added 2 commits September 12, 2018 13:08

Merge remote-tracking branch '2_upstream/master' into vis-less-to-sass

088bd28

Updating visualize_print.pdf baseline

9ecb07b

walterra requested changes Sep 13, 2018

View reviewed changes

Fix ML wizard classes

2366f91

walterra approved these changes Sep 14, 2018

View reviewed changes

cchaos added 2 commits September 14, 2018 11:30

Merge remote-tracking branch '2_upstream/master' into vis-less-to-sass

e7c5722

# Conflicts: # src/core_plugins/kibana/public/visualize/editor/styles/_editor.less # src/ui/public/styles/variables/for-theme.less

removing nesting-indicator sass

23aa72d

cchaos merged commit 9c83f81 into elastic:master Sep 14, 2018

This was referenced Sep 14, 2018

[6.x] Visualize LESS to SASS (#22679) #23197

Merged

Vis LESS to SASS (cont.) #23199

Merged

cchaos deleted the vis-less-to-sass branch October 4, 2018 15:23

timroes mentioned this pull request Oct 4, 2018

Remove old visualization icons #23831

Merged

Conversation

cchaos commented Sep 4, 2018 • edited by timroes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Process taken

Enhancements

1. Fixed the responsive layout to actually show the visualization on small screens

2. Updated the chart type images to use the newly created EUI icons

Questions

Dev Docs

Visualization icons

Uh oh!

trevan commented Sep 4, 2018

Uh oh!

cchaos commented Sep 4, 2018

Uh oh!

elasticmachine commented Sep 4, 2018

💔 Build Failed

Uh oh!

cchaos commented Sep 4, 2018

Uh oh!

cchaos commented Sep 4, 2018

Uh oh!

elasticmachine commented Sep 5, 2018

💔 Build Failed

Uh oh!

trevan commented Sep 5, 2018

Uh oh!

timroes commented Sep 5, 2018

Uh oh!

timroes Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

timroes commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 5, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 6, 2018

💔 Build Failed

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

snide Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos commented Sep 6, 2018

Uh oh!

elasticmachine commented Sep 11, 2018

💔 Build Failed

Uh oh!

chrisdavies commented Sep 12, 2018

Uh oh!

cchaos commented Sep 12, 2018

Uh oh!

cchaos commented Sep 12, 2018

Uh oh!

elasticmachine commented Sep 12, 2018

💚 Build Succeeded

Uh oh!

cchaos commented Sep 12, 2018

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

walterra Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

cchaos Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 13, 2018

💚 Build Succeeded

Uh oh!

timroes commented Sep 14, 2018

Uh oh!

cchaos commented Sep 4, 2018 •

edited by timroes

Loading

timroes commented Sep 5, 2018 •

edited

Loading