(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):
It'd be good to fix both of these issues.
It seems like this might be somewhat easy as well, so tentatively tagging Novice.
Comment | File | Size | Author |
---|---|---|---|
#41 | Screen Shot 2016-12-21 at 11.23.21 AM.png | 30.52 KB | SKAUGHT |
#37 | interdiff-2793207 -28-36.txt | 599 bytes | SKAUGHT |
#37 | 0001-36-limit-scope-of-throbber-position.patch | 1.64 KB | SKAUGHT |
Comments
Comment #2
webchickComment #3
Mukeysh CreditAttribution: Mukeysh as a volunteer and commentedAdded patch for above issue. Attached screenshot for reference.
Comment #4
Mukeysh CreditAttribution: Mukeysh as a volunteer and commentedComment #5
Mukeysh CreditAttribution: Mukeysh as a volunteer and at gai Technologies Pvt Ltd commentedComment #6
laxman.ghavte CreditAttribution: laxman.ghavte as a volunteer and at Faichi Solutions Pvt Ltd for Faichi Solutions Pvt Ltd commentedIssue is in throbber-active.gif image. GIF image has white jagged background.
Comment #7
webchickIf that's not easy to fix (like it requires creating a new image) we can spin-off a follow-up for that.
Comment #8
SKAUGHTit'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.
Comment #9
SKAUGHTComment #10
SKAUGHTtkoleary 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.
Comment #11
SKAUGHTComment #12
SKAUGHTRTL fix included.
Comment #13
bendev CreditAttribution: bendev at WebstanZ commentedtested patch #12 successfully on chrome (and FF)
Comment #14
bendev CreditAttribution: bendev at WebstanZ commentedComment #15
bendev CreditAttribution: bendev at WebstanZ commentedComment #16
scuba_flyI will review this @ DrupalCon Dublin 2016
Comment #17
scuba_fly#12 looks good in firefox, chrome and safari!
Comment #18
scuba_flyComment #19
alexpottI'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).
Comment #20
SKAUGHTI 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.
Comment #21
scuba_flyI 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.
Comment #22
SKAUGHTresetting tags. now that 8.2 is released
Comment #23
xjmI 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.
Comment #24
star-szrSince 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.
Minor: Missing space after colon.
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...
This should have an
/* LTR */
comment. https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...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.Comment #25
yogeshmpawarchanges done as per comment #24 & rerolled the patch.
Comment #26
SKAUGHT#25 seem good. although is missing interdiff
Comment #27
star-szrThanks, looking better! I gave this a manual test and poked around the CSS some more.
I think we should add these back, otherwise we're adding extra space, 2px extra from the borders.
Add
box-sizing: border-box
here.If we use border-box on both elements then I think top needs to be -10px, right 26px, left 26px.
Comment #28
yogeshmpawarUpdated the patch as per comment #27.
Comment #29
star-szr@Yogesh Pawar, can you provide an interdiff please?
Comment #30
yogeshmpawar@Cottser - yes, uploading the interdiff also.
Comment #31
tkoleary CreditAttribution: tkoleary at Acquia commentedLooks great!
Comment #32
xjmThanks @Yogesh Pawar! Assigning to @Cottser in case he has a chance to review again.
Comment #33
alexpottThis CSS seems far too generic to be part of block-place.css. How come it is not prefix by a block place class?
Comment #34
star-szrYes, that does seem problematic. Thanks @alexpott :)
Comment #35
star-szrUnless 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.
Comment #36
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #37
SKAUGHTsorry, oversight. and hopefully not to out of practice making patches (:
Comment #38
SKAUGHTComment #39
tkoleary CreditAttribution: tkoleary at Acquia commentedLooks good!
Comment #40
alexpott@tkoleary if it looks good would you mind sharing a screenshot since there have been several changes to the CSS since the last one.
Comment #41
SKAUGHTvisually, no real change since #12.. but of course a fresh grab never hurts.
Comment #42
SKAUGHTComment #43
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #47
xjmGood 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!