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

Ajax loader spinner #42

Merged
merged 6 commits into from
Dec 22, 2015
Merged

Conversation

e-kolpakov
Copy link
Contributor

Description: this PR adds loader spinners to elements that trigger ajax request.

Testing instructions:

  1. Loader spinner is added to initial DnD view (the one with "Loading drag and drop exercise")
  2. Loader spinner is added to an item when it is dragged into a zone (both correct and incorrect), and removed from it when ajax request finishes

right: 0;
top: 0;
margin-right: 5px;
padding-top: 6px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e-kolpakov Any specific advantage(s) of using padding-top instead of margin-top here? (Just asking because I'm curious :))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsjeyd the spinner is shown as if it is inside the text input, so it's just for the symmetry: both spinner wrapper and text input have the same padding.

@itsjeyd
Copy link
Contributor

itsjeyd commented Dec 21, 2015

@e-kolpakov The spinners work well, and aside from the things I pointed out above the code LGTM as well. I like the idea of showing spinners right where users would be focusing their attention when dropping an item or submitting an additional answer! The only aspect I'm not entirely convinced about is how they behave for items that contain images instead of text -- it looks a bit weird to have the spinner displayed above the image. Do you think it would be possible to display the spinner on top of the image instead (sort of like an overlay)?

@mtyaka
Copy link
Contributor

mtyaka commented Dec 21, 2015

@e-kolpakov Instead of calling show/hide on the spinner div imperatively, the more correct virtual dom inspired approach would be to set a flag in the state (state.items[item_id].xhr_active = true), and then show/hide the spinner div inside view.js based on the xhr_active flag; something like:

h('div.spinner-wrapper', {style: {display: item.xhr_active ? 'block' : 'none'}}, [...])

In fact, we already have item.submitting_location and item.submitting_input that are supposed to be true when the xhr request that submits the location or input data is active, so at least in theory item.submitting_location || item.submitting_input should return true when any xhr requests related to the item are in progress and false otherwise.

@itsjeyd
Copy link
Contributor

itsjeyd commented Dec 22, 2015

@e-kolpakov Looking great, thanks for the additional changes! 👍 from me once you remove that leftover comment. But since I don't know virtualdom that well, I'll let @mtyaka give the final go-ahead for merging.

@mtyaka
Copy link
Contributor

mtyaka commented Dec 22, 2015

@e-kolpakov Thanks for the changes, they look good! 👍 once you get the test passing.

e-kolpakov added a commit that referenced this pull request Dec 22, 2015
@e-kolpakov e-kolpakov merged commit c90e857 into openedx:master Dec 22, 2015
@e-kolpakov e-kolpakov deleted the ajax_loader_spinner branch December 22, 2015 12:41
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.

3 participants