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

Put sass into repo directory #1409

Merged
merged 1 commit into from
Oct 9, 2021
Merged

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Oct 7, 2021

The only thing, which I does not complete for me now is SCSS. When using @use "button-mixins";, it just shows that it cannot find the file.

This PR moves ts/sass/ into sass. With this, it Svelte can find the SASS files. It also reintroduced absolute paths in Svelte components. In this PR / comment, I mentioned that it would be nice if add-on devs could include our SCSS files, and they can only do that if we use relative paths, however that does not include Svelte files, only files in sass/. This means:

  • We should use relative paths in sass/, so they can included by add-on devs
  • We should use absolute paths in any svelte components, so we can see errors.

@dae
Copy link
Member

dae commented Oct 8, 2021

I tested after a bazel clean and it failed:

dae@dip:dtop/ts/deck-options% bazel build ...
INFO: Invocation ID: 18ea0016-ef30-4ca4-adcb-3ac1d678ad43
INFO: Analyzed 61 targets (0 packages loaded, 0 targets configured).
INFO: Found 61 targets...
INFO: From SassCompiler ts/deck-options/deck-options-base.css:
Error: Can't find stylesheet to import.
   ╷
22 │ @import "bootstrap/scss/bootstrap-reboot";
   │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  sass/base.scss 22:9                         @import
  ts/deck-options/deck-options-base.scss 8:9  root stylesheet

Changing the paths in compile_sass.bzl to "sass" seems to resolve it, though if we want absolute paths, it would presumably have to be:

            ".",
            "external/ankidesktop",

When you say "absolute paths", do you mean something like this?

            @use "/sass/scrollbar";

Without the leading '/', I couldn't get 'jump to definition' to work across separate folders/packages, and it seems as if vscode does not expose a way to adjust the sass search path. But when I make that change, the build fails:

6 │ @use "/sass/scrollbar";
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  ts/deck-options/deck-options-base.scss 6:1  root stylesheet

Or do you mean just "sass/scrollbar"? That doesn't seem to work for me - are you using some sass plugin in vscode?

With absolute paths we also need to fix the bootstrap imports:

22 │ @import "bootstrap/scss/bootstrap-reboot";
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The full path would be "sass/bootstrap/scss/bootstrap-reboot" now.

On a vaguely related note, it looks like we could skip the bootstrap copy step and use npm_sass_library() instead:

diff --git a/sass/BUILD.bazel b/sass/BUILD.bazel
index a35d13a5f..a313196f0 100644
--- a/sass/BUILD.bazel
+++ b/sass/BUILD.bazel
@@ -1,4 +1,4 @@
-load("@io_bazel_rules_sass//:defs.bzl", "sass_library")
+load("@io_bazel_rules_sass//:defs.bzl", "npm_sass_library", "sass_library")
 
 sass_library(
     name = "base_lib",
@@ -9,7 +9,10 @@ sass_library(
         "bootstrap-dark.scss",
     ],
     visibility = ["//visibility:public"],
-    deps = ["//sass/bootstrap", "//sass/codemirror"],
+    deps = [
+        "//sass:bootstrap",
+        "//sass/codemirror",
+    ],
 )
 
 sass_library(
@@ -62,6 +65,14 @@ sass_library(
     visibility = ["//visibility:public"],
 )
 
+npm_sass_library(
+    name = "bootstrap",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@npm//bootstrap",
+    ],
+)
+
 exports_files(
     ["_vars.scss"],
     visibility = ["//visibility:public"],
diff --git a/sass/bootstrap/BUILD.bazel b/sass/bootstrap/BUILD.bazel
deleted file mode 100644
index 5dda9e78d..000000000
--- a/sass/bootstrap/BUILD.bazel
+++ /dev/null
@@ -1,22 +0,0 @@
-load("//ts:vendor.bzl", "pkg_from_name", "vendor_js_lib")
-load("@io_bazel_rules_sass//:defs.bzl", "sass_library")
-
-# copy bootstrap sass files in
-vendor_js_lib(
-    name = "sass-sources",
-    include = [
-        "scss",
-    ],
-    base = "external/npm/node_modules/bootstrap/",
-    pkg = pkg_from_name("bootstrap"),
-    visibility = ["//visibility:private"],
-)
-
-# wrap them in a library
-sass_library(
-    name = "bootstrap",
-    srcs = [
-        ":sass-sources",
-    ],
-    visibility = ["//visibility:public"],
-)
diff --git a/ts/change-notetype/BUILD.bazel b/ts/change-notetype/BUILD.bazel
index 41ecbf6c6..7e2a4c440 100644
--- a/ts/change-notetype/BUILD.bazel
+++ b/ts/change-notetype/BUILD.bazel
@@ -12,8 +12,8 @@ compile_sass(
     visibility = ["//visibility:public"],
     deps = [
         "//sass:base_lib",
+        "//sass:bootstrap",
         "//sass:scrollbar_lib",
-        "//sass/bootstrap",
     ],
 )
 
@@ -72,7 +72,7 @@ svelte_check(
         "*.svelte",
     ]) + [
         "//sass:button_mixins_lib",
-        "//sass/bootstrap",
+        "//sass:bootstrap",
         "@npm//@types/bootstrap",
         "@npm//@types/lodash-es",
         "@npm//@types/marked",
diff --git a/ts/components/BUILD.bazel b/ts/components/BUILD.bazel
index fa706524f..e73726a88 100644
--- a/ts/components/BUILD.bazel
+++ b/ts/components/BUILD.bazel
@@ -34,7 +34,7 @@ svelte_check(
         "//sass:base_lib",
         "//sass:button_mixins_lib",
         "//sass:scrollbar_lib",
-        "//sass/bootstrap",
+        "//sass:bootstrap",
         "@npm//@types/bootstrap",
         "//ts/sveltelib:sveltelib_pkg",
     ],
diff --git a/ts/deck-options/BUILD.bazel b/ts/deck-options/BUILD.bazel
index f547d5a9e..d760df5c4 100644
--- a/ts/deck-options/BUILD.bazel
+++ b/ts/deck-options/BUILD.bazel
@@ -13,8 +13,8 @@ compile_sass(
     visibility = ["//visibility:public"],
     deps = [
         "//sass:base_lib",
+        "//sass:bootstrap",
         "//sass:scrollbar_lib",
-        "//sass/bootstrap",
     ],
 )
 
@@ -75,7 +75,7 @@ svelte_check(
     ]) + [
         "//sass:button_mixins_lib",
         "//sass:night_mode_lib",
-        "//sass/bootstrap",
+        "//sass:bootstrap",
         "@npm//@types/bootstrap",
         "@npm//@types/lodash-es",
         "@npm//@types/marked",
diff --git a/ts/editor/BUILD.bazel b/ts/editor/BUILD.bazel
index 5fea49469..fd6e4cedd 100644
--- a/ts/editor/BUILD.bazel
+++ b/ts/editor/BUILD.bazel
@@ -28,8 +28,8 @@ compile_sass(
     group = "local_css",
     visibility = ["//visibility:public"],
     deps = [
+        "//sass:bootstrap",
         "//sass:button_mixins_lib",
-        "//sass/bootstrap",
     ],
 )
 
@@ -83,7 +83,7 @@ svelte_check(
         "*.svelte",
     ]) + [
         "//sass:button_mixins_lib",
-        "//sass/bootstrap",
+        "//sass:bootstrap",
         "@npm//@types/bootstrap",
         "//ts/components",
         "@npm//@types/codemirror",

@hgiesel
Copy link
Contributor Author

hgiesel commented Oct 8, 2021

Should compile now.

However using npm_sass_library gave me this error:

INFO: From SassCompiler ts/editor/fields.css:
Error: Can't find stylesheet to import.

22 │ @import "bootstrap/scss/bootstrap-reboot";
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

sass/base.scss 22:9 @use
ts/editor/fields.scss 4:1 root stylesheet

I guess using npm_sass_library the files are not copied to bazel-bin anymore, which breaks the relative imports within /sass

(I will Squash)

@hgiesel hgiesel force-pushed the stylingcompletions branch from 4dc62f4 to 3101a57 Compare October 8, 2021 11:15
@hgiesel
Copy link
Contributor Author

hgiesel commented Oct 8, 2021

By "absolute" paths I meant relative to base directory, so sass/file. For me on VS Code, the "Go to definition" only works in the sass directory.

In Svelte files, VS Code does not even recognize them as linked files for me. Regarding .scss files in /ts, most of them I would like to refactor into .svelte files or into libs in sass/ anyway.

@dae
Copy link
Member

dae commented Oct 9, 2021

Thanks Henrik, looks good. Might explore the npm thing a bit more later, but it's not a big deal if we need to stick to copying.

@dae dae merged commit c64bac5 into ankitects:main Oct 9, 2021
RumovZ added a commit to RumovZ/anki that referenced this pull request Oct 12, 2021
dae pushed a commit that referenced this pull request Oct 14, 2021
* Only collect card stats on the backend ...

... instead of rendering an HTML string using askama.

* Add ts page Card Info

* Update test for new `col.card_stats()`

* Remove obsolete CardStats code

* Use new ts page in `CardInfoDialog`

* Align start and end instead of left and right

Curiously, `text-align: start` does not work for `th` tags if assigned
via classes.

* Adopt ts refactorings after rebase

#1405 and #1409

* Clean up `ts/card-info/BUILD.bazel`

* Port card info logic from Rust to TS

* Move repeated field to the top

#1414 (comment)

* Convert pseudo classes to interfaces

* CardInfoPage -> CardInfo

* Make revlog in card info optional

* Add legacy support for old card stats

* Check for undefined instead of falsy

* Make Revlog separate component

* drop askama dependency (dae)

* Fix nightmode for legacy card stats
@hgiesel hgiesel deleted the stylingcompletions branch December 31, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants