added sass lint to Canvas #43410

andreadelrio · 2019-08-15T21:04:23Z

Summary

Add sass lint to the Canvas directory

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

snide

Some small comments. Just checked the code and didn't do a browser pass. Maybe @ryankeairns can help there. He's likely the most familiar with canvas.

snide · 2019-08-15T21:15:01Z

x-pack/legacy/plugins/canvas/public/components/autocomplete/autocomplete.scss

-.autocompleteItems, .autocompleteReference {
+.autocompleteItems,
+.autocompleteReference {
+  @include euiScrollBar;
  height: 258px;


Did it not yell about the pixel value here?

Nope, it's not giving any errors for px values. Maybe it needs to be configured or something?

I thought for sure I set this up. I guess not. We don't have it on in EUI either. Something we can look into in a later PR.

https://github.com/sasstools/sass-lint/blob/master/docs/rules/variable-for-property.md

snide · 2019-08-15T21:16:07Z

x-pack/legacy/plugins/canvas/public/components/border_resize_handle/border_resize_handle.scss

-  box-shadow: 0 2px 2px -1px rgba(153, 153, 153, 0.3), 0 1px 5px -2px rgba(153, 153, 153, 0.3);
+  background-color: $euiColorEmptyShade;
+  border: 1px solid $euiColorDarkShade;
+  box-shadow: 0 2px 2px -1px transparentize($euiColorMediumShade, .7), 0 1px 5px -2px transparentize($euiColorMediumShade, .7);


Not 100% sure what this is used for, but a reminder we have a bunch of shadow mixins. They may or may not be a good sub.

https://github.com/elastic/eui/blob/master/src/global_styling/mixins/_shadow.scss

snide · 2019-08-15T21:17:47Z

x-pack/legacy/plugins/canvas/public/components/fullscreen/fullscreen.scss

  .canvasLayout {
-    background: #000; // This hex is OK, we always want it black
+    background: #000; // sass-lint:disable-line no-color-literals


$euiColorInk will give you this. It's always black.

x-pack/legacy/plugins/canvas/public/components/page_manager/page_manager.scss

snide · 2019-08-15T21:20:50Z

.sass-lint.yml

@@ -6,6 +6,7 @@ files:
    - 'src/legacy/ui/public/vislib/**/*.s+(a|c)ss'
    - 'x-pack/legacy/plugins/rollup/**/*.s+(a|c)ss'
    - 'x-pack/legacy/plugins/security/**/*.s+(a|c)ss'
+    - 'x-pack/legacy/plugins/canvas/**/*.s+(a|c)ss'


snide · 2019-08-15T21:23:11Z

Also @andreadelrio. I notice a bunch of binary files changed? Any idea what that's about?

andreadelrio · 2019-08-15T21:24:20Z

Also @andreadelrio. I notice a bunch of binary files changed? Any idea what that's about?

Hm no, no idea. I only touched scss files.

elasticmachine · 2019-08-15T22:14:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-15T23:28:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ryankeairns

This is looking much cleaner, thanks for tidying up!

Just a few comments below, hopefully they are all quick fixes. You'll also need to remove those 3 binary zip files from this change. Let me know if you need help with that. Thanks!

x-pack/legacy/plugins/canvas/public/components/fullscreen/fullscreen.scss

x-pack/legacy/plugins/canvas/public/style/main.scss

ryankeairns

👍 LGTM. Thanks for addressing all the feedback.

elasticmachine · 2019-08-16T18:38:53Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-08-16T18:57:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

snide

LGTM. Don't forget to backport once you merge.

elasticmachine · 2019-08-16T20:29:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Added sass lint to Canvas

…_update_json_spec * 'master' of github.com:elastic/kibana: (35 commits) fix: 🐛 pass whole action context to isCompatible() method (elastic#43457) Deleted old kbn-top-nav directive (elastic#43168) [ML] Fixing cloning of single metric distinct count job (elastic#43435) Update @elastic/charts version 8.1.6 > 9.1.1 (elastic#43516) [Inspector Views] [Request View] - Migrate inspector_views to new platform (elastic#43191) [ML] Adding loading indicators to all wizard charts (elastic#43382) disable flaky test (elastic#43492) feature(code/frontend): cancel file blob and directory commits request if outdated (elastic#43348) fix(code/frontend): button group url should have previous query string (elastic#43428) [SIEM] Fixes index substring incorrectly matching configured indices and failing to install ML job (elastic#43409) [SIEM] Adds performance enhancements such by removing wasted renderers and adding incremental DOM rendering (elastic#43157) disable flaky test (elastic#37859) Added sass lint to Canvas (elastic#43410) [Maps] add indicator when layer is filtered by search bar (elastic#43283) Properly validate current user password during password change. (elastic#43447) Spaces - allow for hex color codes that include uppercase characters (elastic#43470) [Reporting] Add a bit more logging and a few more logging level promotions (elastic#43415) Partially convert index pattern server to typescript (elastic#43291) [Infra UI] Use sum for aggregating AWS metrics. (elastic#43293) [SIEM] Format bytes columns in timeline (elastic#43147) ...

added sass lint to Canvas

3bb6736

andreadelrio requested a review from a team as a code owner August 15, 2019 21:04

andreadelrio added the EUI label Aug 15, 2019

snide reviewed Aug 15, 2019

View reviewed changes

small fixes

90a7f9e

andreadelrio force-pushed the sass-lint-canvas branch from ebd6f5b to 90a7f9e Compare August 15, 2019 22:20

ryankeairns requested changes Aug 16, 2019

View reviewed changes

x-pack/legacy/plugins/canvas/public/components/fullscreen/fullscreen.scss Outdated Show resolved Hide resolved

x-pack/legacy/plugins/canvas/public/style/main.scss Outdated Show resolved Hide resolved

x-pack/legacy/plugins/canvas/public/style/main.scss Outdated Show resolved Hide resolved

replacing color variable for white

eeb25ed

andreadelrio force-pushed the sass-lint-canvas branch from 7a4512e to eeb25ed Compare August 16, 2019 17:55

ryankeairns self-requested a review August 16, 2019 18:01

ryankeairns approved these changes Aug 16, 2019

View reviewed changes

andreadelrio added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Aug 16, 2019

andreadelrio added v7.4.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 16, 2019

Merge branch 'master' of github.com:elastic/kibana into sass-lint-canvas

84934c4

snide approved these changes Aug 16, 2019

View reviewed changes

andreadelrio merged commit dbab52a into elastic:master Aug 16, 2019

andreadelrio mentioned this pull request Aug 16, 2019

[7.x] Added sass lint to Canvas (#43410) #43497

Merged

andreadelrio added a commit to andreadelrio/kibana that referenced this pull request Aug 16, 2019

Added sass lint to Canvas (elastic#43410)

a330153

Added sass lint to Canvas

andreadelrio added a commit that referenced this pull request Aug 16, 2019

Added sass lint to Canvas (#43410) (#43497)

471bae8

Added sass lint to Canvas

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added sass lint to Canvas #43410

added sass lint to Canvas #43410

andreadelrio commented Aug 15, 2019

snide left a comment

snide Aug 15, 2019

andreadelrio Aug 15, 2019

snide Aug 15, 2019

snide Aug 15, 2019

snide Aug 15, 2019

snide Aug 15, 2019

snide commented Aug 15, 2019

andreadelrio commented Aug 15, 2019

elasticmachine commented Aug 15, 2019

elasticmachine commented Aug 15, 2019

ryankeairns left a comment

ryankeairns left a comment

elasticmachine commented Aug 16, 2019

elasticmachine commented Aug 16, 2019

snide left a comment

elasticmachine commented Aug 16, 2019

added sass lint to Canvas #43410

added sass lint to Canvas #43410

Conversation

andreadelrio commented Aug 15, 2019

Summary

Checklist

snide left a comment

Choose a reason for hiding this comment

snide Aug 15, 2019

Choose a reason for hiding this comment

andreadelrio Aug 15, 2019

Choose a reason for hiding this comment

snide Aug 15, 2019

Choose a reason for hiding this comment

snide Aug 15, 2019

Choose a reason for hiding this comment

snide Aug 15, 2019

Choose a reason for hiding this comment

snide Aug 15, 2019

Choose a reason for hiding this comment

snide commented Aug 15, 2019

andreadelrio commented Aug 15, 2019

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

ryankeairns left a comment

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2019

elasticmachine commented Aug 16, 2019

💚 Build Succeeded

snide left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2019

💚 Build Succeeded