added sass lint to Canvas by andreadelrio · Pull Request #43410 · elastic/kibana

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 {
+  @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

+  // This hex is OK, we always want it black
  .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

    - '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 t// 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

Conversation

andreadelrio commented Aug 15, 2019

Summary

Checklist

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

snide Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

andreadelrio Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

snide Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

snide Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

snide Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snide Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

snide commented Aug 15, 2019

Uh oh!

andreadelrio commented Aug 15, 2019

Uh oh!

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 16, 2019

Uh oh!

elasticmachine commented Aug 16, 2019

💚 Build Succeeded

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 16, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants