Introduce bootstrap's spinner as throbber [#3047339]

Problem/Motivation

As discussed in #3024839: Update to Bootstrap v4.3.1 we want to introduce the (growing, my preference) spinner as throbber (aka ajax-progress).

This is not always wanted/needed so we introduce a new theme setting, which allows to use it "Never" (off) or "Frontend only" (on).

We introduce a third option for using it "always", as the theme can only inject/override libraries if active this to be done in bs_lib.

Regarding the design, I thought of using the primary color for the background to make the throbber look familiar. And I added a border and box-shadow in the currentColor (which is used by bootstrap for the background) to ensure good visibility everywhere.

Dependencies

Needs the new component from here #3047335: Provide new component for spinner / ajax-loading.
When releasing it needs to be done together with bs_lib.

Comment	File	Size	Author
#12	3047339-introduce_spinner-12.patch	13.71 KB	ytsurk
#2	3047339-introduce_spinner-2.patch	7.17 KB	ytsurk

Comments

Comment #1

11 April 2019 at 07:17

ytsurk created an issue. See original summary.

Comment #2

ytsurk

Switzerland

commented 11 April 2019 at 07:19

Status	File	Size
new	3047339-introduce_spinner-2.patch	7.17 KB

Comment #3

pivica commented 11 April 2019 at 10:37

Did a quick code check, nothing special for now. Need still to test it locally. Can we get some animated gif screenshots of how this works :)?

+++ b/themes/bs_bootstrap/sass/components/partials/_ajax-progress.scss
@@ -0,0 +1,58 @@
+      border: $spinner-border-width-sm solid currentColor;

Didn't know about this, cool.

+++ b/themes/bs_bootstrap/sass/components/partials/_ajax-progress.scss
@@ -0,0 +1,58 @@
+  }

I would add one empty line here after }.

+++ b/themes/bs_bootstrap/sass/components/partials/_ajax-progress.scss
@@ -0,0 +1,58 @@
+.ui-dialog:not(.ui-dialog-off-canvas) .ajax-progress-throbber {

You can put this above and just write it as

.ajax-progress {
  &-throbber {
    ui-dialog:not(.ui-dialog-off-canvas) & {
      ...
    }

But maybe you don't want to group it like this. Fine to leave it like this also.

Comment #4

pivica commented 11 April 2019 at 10:54

Status:

Needs review

» Needs work

> In #3047335: Use spinner as throbber "always" option we introduce a third option for using it "always", as the theme can only inject/override libraries if active.

Wondering should we maybe move this setting to bs_lib?

Thinking about this more spinner is actually a new Bootstrap component and it makes sense we maybe define it in bs_lib.libraries.yml.

In bs_bootstrap we can then libraries-extend: bs_lib spinner library and attach additional CSS code we are generating in bs_bootstrap.
We need to make sure that the drush generator is handling libraries-extend properly, not sure currently it is covering that case. If this is a case then we can use libraries-override which generator does support, and inject maybe dependency to our CSS or do some library hook alter, not sure.

I also see two spinner options - border and growing, should we make a theme option for this also because of this change injected CSS class?

Comment #5

pivica commented 11 April 2019 at 10:55

@ytsurk not sure about all these proposals i did, would need to sit and code it a bit to get a sense first... What do you think which path is better?

Comment #6

pivica commented 11 April 2019 at 10:58

> In bs_bootstrap we can then libraries-extend: bs_lib spinner library and attach additional CSS code we are generating in bs_bootstrap...

Actually, i guess approach we are using for all other components would work here also like for example (bs_bootsrap.info.yml):

libraries-override:
  bs_lib/close:
    css:
      component:
        /libraries/bootstrap/dist/css/bootstrap.css: css/components/close.css

and do exactly the same stuff for a spinner.

Comment #7

ytsurk

Switzerland

commented 11 April 2019 at 12:45

Thx for feedback! I'll re-roll soon.

I did not use libraries-extend or -overwrite, because we're more flexible in adding it where it is needed.

I also wouldn't move the library to bs_base, so we only need to compile SASS in one place.

We can also introduce a theme setting for the style. Right now it would be border or growing (maybe also growing with border/box-shadow).

Comment #8

pivica commented 11 April 2019 at 13:07

> I also wouldn't move the library to bs_base, so we only need to compile SASS in one place.

We always want to compile SASS in child themes no matter what because variables override, etc.

> I also wouldn't move the library to bs_base, so we only need to compile SASS in one place.

I was thinking bs_lib, not bs_base. If you take a look at bs_lib.libraries.yml file you will see component Bootstrap library definitions there. It makes sense we expand this with a spinner and other new libraries when they are added in new Bootstrap versions.