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

Visualize LESS to SASS #22679

Merged
merged 20 commits into from
Sep 14, 2018
Merged

Visualize LESS to SASS #22679

merged 20 commits into from
Sep 14, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 4, 2018

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

Process taken

  1. 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.
  2. 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.
  3. 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.
  4. 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.

screen shot 2018-09-04 at 16 00 54 pm

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

  1. I can't find a plain <visualize /> element anywhere, though there are CSS selectors for it. Is this an outdated selector?
  2. 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).

cchaos added 7 commits August 29, 2018 10:54
- 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
@cchaos cchaos requested a review from snide September 4, 2018 18:26
@cchaos 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
@trevan
Copy link
Contributor

trevan commented Sep 4, 2018

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 4, 2018

@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
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor Author

cchaos commented Sep 4, 2018

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 4, 2018

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@trevan
Copy link
Contributor

trevan commented Sep 5, 2018

@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 timroes requested a review from walterra September 5, 2018 08:28
@timroes
Copy link
Contributor

timroes commented Sep 5, 2018

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

@timroes timroes self-requested a review September 5, 2018 08:28
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

timroes commented Sep 5, 2018

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.

Tim Roes and others added 4 commits September 5, 2018 11:21
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

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.

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

Choose a reason for hiding this comment

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

would this just be --paginatedSelectable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 6, 2018

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

# Conflicts:
#	src/core_plugins/input_control_vis/public/register_vis.js
@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisdavies
Copy link
Contributor

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
Copy link
Contributor Author

cchaos commented Sep 12, 2018

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
Copy link
Contributor Author

cchaos commented Sep 12, 2018

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
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor Author

cchaos commented Sep 12, 2018

Tests are passing!!

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

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

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

@@ -10,7 +10,7 @@ <h3 class="kuiTitle kuiVerticalRhythm">From a New Search, Select Index</h3>
list="indexPatterns"
list-property="attributes.title"
user-make-url="withIndexPatternUrl"
class="wizard-row visualizeWizardPaginatedSelectableList kuiVerticalRhythm"
class="wizard-row visWizard__paginatedSelectableList kuiVerticalRhythm"
Copy link
Contributor

Choose a reason for hiding this comment

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

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;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor

timroes commented Sep 14, 2018

@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
Copy link
Contributor

@cchaos Latest ML changes LGTM!

@timroes
Copy link
Contributor

timroes commented Sep 14, 2018

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

@cchaos
Copy link
Contributor Author

cchaos commented Sep 14, 2018

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
Copy link
Contributor

💚 Build Succeeded

@cchaos cchaos merged commit 9c83f81 into elastic:master Sep 14, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos cchaos deleted the vis-less-to-sass branch October 4, 2018 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants