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

Add book tags submission indicator #5705

Merged

Conversation

jimchamp
Copy link
Collaborator

Closes #5646

Adds the submission indicator from #4948 to the book tags Vue component.

Technical

Adds a new Vue component representing a submission status indicator. A spinner is displayed when a tag is POSTed to the server. A success or failure message is displayed when the response has been received.

State is managed by an object imported from ObservationService.js (I'm not thrilled about this, but it saves a bunch of difficult to follow messaging between components). The object's state is changed by the updateObservation function, which is used to POST observations to the server.

Testing

Screenshot

loading_indicator

Stakeholders

@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Sep 30, 2021
@RayBB
Copy link
Collaborator

RayBB commented Oct 1, 2021

Functionally, I think this serves the purpose of what I was hoping for.
Personally I think it would be better to put the status at the bottom rather than the middleish of modal. Something like this.

Additionally, again my own design preference, I think it is information overload to show a spinning loading indicator after every single change. It is a little distracting and for some maybe anxiety producing. Others also have done away with loading indicators.

I think we could simplify by not revealing the state of saving. Just have the states of saved or failed. Perhaps a nice indicator of a new save would be to make a checkmark bounce or redraw or something. I'll think on it more.

@jimchamp

@jimchamp
Copy link
Collaborator Author

jimchamp commented Oct 1, 2021

This is great feedback! Having only success and failure states removes the need for an object to track state. I also like the idea of using animations to indicate state.

@RayBB
Copy link
Collaborator

RayBB commented Oct 1, 2021

One more thing I forgot to mention in that message is accessibility.
I tried to search briefly but didn't see a whole lot about how do we make this easy for people to understand that are using screen readers? I think putting it at the bottom can help but I don't know if there's something else to do there

@mekarpeles mekarpeles self-assigned this Oct 4, 2021
@mekarpeles mekarpeles added the Priority: 2 Important, as time permits. [managed] label Oct 4, 2021
@mekarpeles
Copy link
Member

I agree that the positioning is kind of off (bottom or top or somewhere it doesn't move the position of other content) would be best.

Also, I agree that the the spinner is kind of distracting.

I might just put the green text "saved" next to the X as that's likely where they're going to go to close the form (and they'll have confidence everything is saved).

@RayBB does these changes seem like a good compromise?

If so, @jimchamp can make these tweaks? Or are they hard to implement?

I'm deferring to you the discretion to either merge as is or make these tweaks if they're easy enough.

Thanks everyone!

@cdrini cdrini added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] and removed Priority: 2 Important, as time permits. [managed] labels Nov 15, 2021
@jimchamp jimchamp force-pushed the 5646/bug/submission-indicator branch from d2f33a3 to c2a1aaa Compare November 18, 2021 23:45
@jimchamp
Copy link
Collaborator Author

I have removed the spinner and added a message explaining that the tags in the first section have been saved.

tag-saved-message

@mekarpeles
Copy link
Member

lgtm, if we're going to say, "the reviews above [...]" then maybe we should just put the copy above, alongside the reviews, but I think this makes the solution "possible", merging

@mekarpeles mekarpeles merged commit de721b4 into internetarchive:master Dec 6, 2021
@mekarpeles
Copy link
Member

Revisiting this for accessibility may be a good one for your Menu for volunteers @jimchamp

@jimchamp jimchamp deleted the 5646/bug/submission-indicator branch January 27, 2022 19:58
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Submission feedback missing from Book Tags modal
4 participants