(I wavered between major and normal on this. It's certainly not remotely a deal-breaker, but it's a very user-facing visual bug that impacts first impressions of this feature.)

When you click one of the plus icons in Place Block module, the plus icon gets offset by kind of a gnarly looking AJAX spinner, at least in Chrome (sorry, this is tricky to take a screenshot of):

Gnarly white spinner

It'd be good to fix both of these issues.

It seems like this might be somewhat easy as well, so tentatively tagging Novice.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick created an issue. See original summary.

webchick’s picture

Mukeysh’s picture

Added patch for above issue. Attached screenshot for reference.

Mukeysh’s picture

Status: Active » Needs review
Mukeysh’s picture

laxman.ghavte’s picture

Status: Needs review » Needs work

Issue is in throbber-active.gif image. GIF image has white jagged background.

webchick’s picture

If that's not easy to fix (like it requires creating a new image) we can spin-off a follow-up for that.

SKAUGHT’s picture

it's odd that throbber gif is so broken here -- it is just core's gif (but no where else is inheriting this issue). something must be resizing it.

SKAUGHT’s picture

SKAUGHT’s picture

tkoleary proposed overlaying the throbber over the plus sign [#2739079] so, here it is. (:

it is a clean patch from dev line. no diff. i think it does need RTL check still

this additional related issue is interesting. just linking.

SKAUGHT’s picture

Status: Needs work » Needs review
SKAUGHT’s picture

bendev’s picture

tested patch #12 successfully on chrome (and FF)

bendev’s picture

Issue tags: -Usability, -CSS, -Novice +Dublin2016
bendev’s picture

Issue tags: +Usability, +CSS, +Novice
scuba_fly’s picture

Assigned: Unassigned » scuba_fly

I will review this @ DrupalCon Dublin 2016

scuba_fly’s picture

Status: Needs review » Reviewed & tested by the community

#12 looks good in firefox, chrome and safari!

scuba_fly’s picture

Assigned: scuba_fly » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'm wondering if this should wait for the efforts going on in #1974928: Update throbber icon since that is going to replace the throbber in Bartik and one of the design considerations should be that it looks good anywhere on the Bartik page. Also the fix being done in this issue is specific to the throbber when used in Bartik so I'm not sure it'd be always the correct fix for all themes. A particularly relevant comment about themes and throbbers is #1974928-118: Update throbber icon.

Setting back to 'needs review' to consider implications and options (instead of postponing).

SKAUGHT’s picture

I think that even with the throbber icon becoming an svg that this patches re-position of the icon should be good pairings. with the assumption that any new throbber is the same size.

individual contrib and custom themes will then only need to deal with any off-positions their own css may introduce (expectable).

it would certainly be nice if this was patched was included for 8.2 and Block Place's initial release.

scuba_fly’s picture

I agree with SKAUGHT.
It makes it look so much nicer for the 8.2 release. And even if with the svg thing we need to change anything we could make a new patch for that.

SKAUGHT’s picture

Version: 8.2.x-dev » 8.2.0
Status: Needs review » Reviewed & tested by the community

resetting tags. now that 8.2 is released

xjm’s picture

Version: 8.2.0 » 8.2.x-dev
Assigned: Unassigned » star-szr
Priority: Major » Normal

I don't think #19 is in scope, or at least I don't think it makes sense to block an obvious visual bugfix for an experimental module on a proposed UI change.

This is not a major bug though IMO, so downgrading and fixing the branch. Also assigning to Cottser, because CSS, and it's probably his call to decide whether @alexpott's proposal makes sense.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Needs work

Since this is inside an experimental module, and the CSS scope is limited to the output from that module, this seems like a reasonable workaround for now. Short of replacing the spinner, of course. However, per @alexpott's comment, it would be good to make sure that what is done here is applicable to more than just Bartik. If this change only makes sense/works in Bartik it probably needs to be refactored to work a bit more generically or moved to Bartik.

  1. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,42 @@
    +  position:relative;
    ...
    +  overflow:visible;
    

    Minor: Missing space after colon.

  2. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,42 @@
    -  font-size: 1rem;
    +  font-size: 1em;
    

    The recommendation for core CSS is rem with a px fallback, so we should at least leave this as is IMO. Unless it was changed for a reason.

    https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

  3. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,42 @@
    +  right: 28px;
    

    This should have an /* LTR */ comment. https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

  4. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,42 @@
    +  background-position: CENTER 6px;
    

    CENTER should be lowercase - I can't find where this is in our coding standards (might not be there) but it would at least be consistent with the rest of core.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

changes done as per comment #24 & rerolled the patch.

SKAUGHT’s picture

Status: Needs review » Reviewed & tested by the community

#25 seem good. although is missing interdiff

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Thanks, looking better! I gave this a manual test and poked around the CSS some more.

  1. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,43 @@
    -  box-sizing: border-box;
    ...
    -  white-space: nowrap;
    

    I think we should add these back, otherwise we're adding extra space, 2px extra from the borders.

  2. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,43 @@
    +.ajax-progress-throbber .throbber {
    

    Add box-sizing: border-box here.

  3. +++ b/core/modules/block_place/css/block-place.css
    @@ -13,17 +13,43 @@
    +  top: -12px;
    +  right: 28px; /* LTR */
    ...
    +  left: 28px;
    

    If we use border-box on both elements then I think top needs to be -10px, right 26px, left 26px.

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
1.27 KB

Updated the patch as per comment #27.

star-szr’s picture

@Yogesh Pawar, can you provide an interdiff please?

yogeshmpawar’s picture

FileSize
1.03 KB

@Cottser - yes, uploading the interdiff also.

tkoleary’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

xjm’s picture

Assigned: Unassigned » star-szr

Thanks @Yogesh Pawar! Assigning to @Cottser in case he has a chance to review again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/block_place/css/block-place.css
@@ -23,7 +24,35 @@
+.ajax-progress-throbber .throbber {

This CSS seems far too generic to be part of block-place.css. How come it is not prefix by a block place class?

star-szr’s picture

Assigned: star-szr » Unassigned

Yes, that does seem problematic. Thanks @alexpott :)

star-szr’s picture

Unless the library referencing this CSS is loaded very selectively, in which case that CSS might be okay. I can't test right now but it looks like that may be the case.

tkoleary’s picture

Status: Needs review » Needs work
SKAUGHT’s picture

SKAUGHT’s picture

Status: Needs work » Needs review
tkoleary’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots

@tkoleary if it looks good would you mind sharing a screenshot since there have been several changes to the CSS since the last one.

SKAUGHT’s picture

visually, no real change since #12.. but of course a fresh grab never hurts.

SKAUGHT’s picture

Status: Needs work » Needs review
tkoleary’s picture

Status: Needs review » Reviewed & tested by the community

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • xjm committed c5cfdf9 on 8.4.x
    Issue #2793207 by SKAUGHT, Yogesh Pawar, Mukeysh, Cottser, webchick,...

  • xjm committed f9d2ac6 on 8.3.x
    Issue #2793207 by SKAUGHT, Yogesh Pawar, Mukeysh, Cottser, webchick,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs screenshots

Good catch on the CSS specificity in #33.

Looking over the patch, it appears that RTL is covered appropriately. I also re-double-checked the pixel counts and also looked at the updated screenshot.

#2775725: Update throbber icon is making good progress, so hopefully a followup for #19 can happen before too much longer. Rather than filing a followup that may or may not be needed, I just added a should-have item to #2739075: [plan] Make Place Blocks module functionality part of the Block module (etc.).

Since it's an alpha experimental module, I went ahead and committed this based on earlier frontend reviews without requiring a final frontend signoff, and I backported this fix to 8.3.x as well. It should be a nice visual cleanup.

Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.