Problem/Motivation

View UI has some custom admin CSS that overrides the throbber styling on views edit pages.

This is a totally different design to the throbbers in the rest of core and it's impossible for other modules to reuse this styling.

Proposed resolution

Add a new option to the throbber API that will conditionally switch the styling to a 'full screen' throbber instead of an inline one.

We should also add this to the Seven style guide and provide guidance on when to use an inline or a full screen throbber.

Remaining tasks

Discuss
Patch

User interface changes

None. Until other modules choose to use the new styling.

API changes

A new parameter called 'fullscreen' to the API

CommentFileSizeAuthor
#29 interdiff-2207695-23-29.txt1.03 KBmitsuroseba
#29 expand-throbber-fullscreen-2207695-29.patch7 KBmitsuroseba
#23 interdiff.txt672 bytessam152
#23 expand-throbber-fullscreen-2207695-23.patch5.93 KBsam152
#21 expand-throbber-fullscreen-2207695-21.patch5.27 KBlewisnyman
#16 interdiff-2207695-15-16.txt1.27 KBmanuel garcia
#16 expand-throbber-fullscreen-2207695-16.patch8.08 KBmanuel garcia
#15 expand-throbber-fullscreen-2207695-15.patch4.71 KBlewisnyman
#14 expand-throbber-fullscreen-2207695-14.patch3.8 KBmanuel garcia
#13 expand-throbber-fullscreen-2207695-13.patch3.91 KBmanuel garcia
#10 interdiff-2207695-6-9.txt1.7 KBpakmanlh
#9 expand-throbber-fullscreen-2207695-9.patch3.84 KBpakmanlh
#6 expand-throbber-fullscreen-2207695-6.patch2.13 KBlewisnyman
#2 expand-throbber-fullscreen-2207695-2.patch1.38 KBpakmanlh
Screen Shot 2014-02-28 at 15.08.52.jpg193.75 KBlewisnyman

Comments

dawehner’s picture

dawehner
Primary language German
commented

There is also some css in dialog.theme.css for it.

pakmanlh’s picture

pakmanlh
he/him
Primary language Catalan
Location Barcelona
commented
StatusFileSize
new expand-throbber-fullscreen-2207695-2.patch1.38 KB

Here are a first approach to implement a fullscreen option inside ajax.js.
I don't find from where is calling the reference to the throbber inside the «views_ui/js/ajax.js» file.
Also I added a link to related issue that pretend modify the throbber style.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Active » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +JavaScript

Status: Needs review » Needs work

The last submitted patch, 2: expand-throbber-fullscreen-2207695-2.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new expand-throbber-fullscreen-2207695-6.patch2.13 KB

I found the other progress call in views preview. We still needs to shift over the CSS, but it would good get some input from views/JS people that we're on the right track with this API change.

pakmanlh’s picture

pakmanlh
he/him
Primary language Catalan
Location Barcelona
commented
Related issues: +#1847916: Replace the ajax-progress-throbber div with a class
nod_’s picture

nod_
Primary language French
Location Lille
commented

JS looks good to me, feel free to RTBC.

pakmanlh’s picture

pakmanlh
he/him
Primary language Catalan
Location Barcelona
commented
StatusFileSize
new expand-throbber-fullscreen-2207695-9.patch3.84 KB

Here I updated the views styles to show the throbber properly, I don't change many things because I think the visual styles belong to the #1847916: Replace the ajax-progress-throbber div with a class related issue.

pakmanlh’s picture

pakmanlh
he/him
Primary language Catalan
Location Barcelona
commented
StatusFileSize
new interdiff-2207695-6-9.txt1.7 KB

I added an interdiff.

lewisnyman’s picture

lewisnyman
he/him
commented

Just a note here, with a patch applied the Views interface should be identical to how it was before.

I'm really happy we can add an option like this to core :-) I think it's going to be really useful in the admin interface.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

Ah so one thing here is that we have to move this styling out of the views css and into system.module.css, under the rest of the throbber CSS.

manuel garcia’s picture

manuel garcia commented
StatusFileSize
new expand-throbber-fullscreen-2207695-13.patch3.91 KB 
$ git apply -v expand-throbber-fullscreen-2207695-9.patch
Checking patch core/misc/ajax.js...
Checking patch core/modules/views/js/ajax_view.js...
Checking patch core/modules/views_ui/css/views_ui.admin.css...
Checking patch core/modules/views_ui/css/views_ui.admin.theme.css...
Hunk #1 succeeded at 1088 (offset 9 lines).
Checking patch core/modules/views_ui/js/ajax.js...
Checking patch core/modules/views_ui/lib/Drupal/views_ui/ViewPreviewFormController.php...
error: core/modules/views_ui/lib/Drupal/views_ui/ViewPreviewFormController.php: No such file or directory

Apparently that stuf is now done inside this file:
core/modules/views_ui/lib/Drupal/views_ui/ViewPreviewForm.php

Rerolled the patch to reflect the change.

manuel garcia’s picture

manuel garcia commented
StatusFileSize
new expand-throbber-fullscreen-2207695-14.patch3.8 KB

Made a tiny mistake in the reroll, here's the good one.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new expand-throbber-fullscreen-2207695-15.patch4.71 KB

Here's an updated patch with the CSS moved out of Views and into system CSS

manuel garcia’s picture

manuel garcia commented
StatusFileSize
new expand-throbber-fullscreen-2207695-16.patch8.08 KB
new interdiff-2207695-15-16.txt1.27 KB

While debugging the throbber in views, I noticed that when you open a dialog is missing its padding, however, when you close the dialog, the same throbber is properly displayed.

Looking into why this was happening, I found out that we have two different CSS rules declaring the same styles, .ajax-progress-fullscreen (with patch #15 inside modules/system/system.module.css), and .ui-dialog .ajax-progress-throbber which is in misc/dialog.theme.css. I suppose we should clean this up in another issue.

As far as why when you open the dialog it does not get its padding, the probelm was too general CSS selectors, which I've modified to target what they need only. This has the bonus of being more efficient for the browser to figure out, and it solves our problem with the padding. See the interdiff.

I've also removed core/modules/views_ui/images/loading-small.gif which is a duplicate image of the one on in /misc.

lewisnyman’s picture

lewisnyman
he/him
commented

@Manuel I think we could fix the problem with the styling by moving where the fullscreen throbber is inserted on the page. We could just append it to the page wrapper, it doesn't need to sit alongside the element.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue tags: +frontend
lewisnyman’s picture

lewisnyman
he/him
commented

16: expand-throbber-fullscreen-2207695-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: expand-throbber-fullscreen-2207695-16.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new expand-throbber-fullscreen-2207695-21.patch5.27 KB

I've rerolled this patch and I've moved the fullscreen throbber to the body. This fixes any positioning issues. Woo!

LewisNyman queued 21: expand-throbber-fullscreen-2207695-21.patch for re-testing.

sam152’s picture

sam152 commented
StatusFileSize
new expand-throbber-fullscreen-2207695-23.patch5.93 KB
new interdiff.txt672 bytes

I tested this and it seemed strange to have a different progress indicator for the arguments preview and the rest of the views UI. Patch updates to address both parts of views.

lewisnyman’s picture

lewisnyman
he/him
commented

I couldn't find any other instances in Views UI that uses the throbber. Looks like we're ready to roll?

LewisNyman queued 23: expand-throbber-fullscreen-2207695-23.patch for re-testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Looks like there are also a few more lines of code we can remove from Views?

/* Base Page */
#views-ui-list-page .ajax-progress-throbber,
.views-admin .ajax-progress-throbber {
  /* Can't do center:50% middle: 50%, so approximate it for a typical window size. */
  left: 49%;
  position: fixed;
  top: 48.5%;
  z-index: 1000;
}
#views-ui-list-page .ajax-progress-throbber .message,
.views-admin .ajax-progress-throbber .message {
  display: none;
}

/* Modal */
#views-ajax-popup .ajax-progress-throbber {
  /* Can't do center:50% middle: 50%, so approximate it for a typical window size. */
  left: 49%;
  position: fixed;
  top: 48.5%;
  z-index: 1000;
}
#views-ajax-popup .ajax-progress-throbber .message {
  display: none;
}

The last submitted patch, 23: expand-throbber-fullscreen-2207695-23.patch, failed testing.

mitsuroseba’s picture

mitsuroseba commented
Assigned: Unassigned » mitsuroseba
mitsuroseba’s picture

mitsuroseba commented
Assigned: mitsuroseba » Unassigned
Status: Needs work » Needs review
StatusFileSize
new expand-throbber-fullscreen-2207695-29.patch7 KB
new interdiff-2207695-23-29.txt1.03 KB

Rerolled and remove some strings from views_ui.admin.css

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

This is looking great now. Happy friday!

The last submitted patch, 13: expand-throbber-fullscreen-2207695-13.patch, failed testing.

The last submitted patch, 14: expand-throbber-fullscreen-2207695-14.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Thanks views for giving so much to core :)

Committed 2896f4a and pushed to 8.x. Thanks!

  • alexpott committed 2896f4a on 8.0.x 
    Issue #2207695 by Manuel Garcia, LewisNyman, pakmanlh, mitsuroseba,...
m1r1k’s picture

m1r1k commented
Issue tags: +#ams2014contest

Status: Fixed » Closed (fixed)

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

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented
Issue tags: -

This is awesome! Finally!

Issue summary states:

We should also add this to the Seven style guide and provide guidance on when to use an inline or a full screen throbber.

Has this been done?

Also, does this new feature deserve a change notice?