nteract / bookstore

📚 Notebook storage and publishing workflows for the masses
https://bookstore.readthedocs.io
BSD 3-Clause "New" or "Revised" License
201 stars 23 forks source link

Improve button #171

Closed mpacer closed 4 years ago

mpacer commented 4 years ago

Closes #170.

This disables the button when clicked and has some style improvements when hovering over the button.

codecov[bot] commented 4 years ago

Codecov Report

Merging #171 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files          10       10           
  Lines         507      507           
=======================================
  Hits          419      419           
  Misses         88       88
codecov[bot] commented 4 years ago

Codecov Report

Merging #171 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files          10       10           
  Lines         507      507           
=======================================
  Hits          419      419           
  Misses         88       88
mpacer commented 4 years ago

Do we need to re-enable the button after the post response? I could see a UI where the clone action doesn't leave the page and you wouldn't be able to re-initiate the clone action. If this isn't a concern at this time the change looks good to me.

Right now the post request is being populated statically from the jinja template and so I am not sure in what situation the first post request would fail when an second (or otherwise additional) post request would succeed.

@mseal Could you describe the case that you're imagining in greater detail?

MSeal commented 4 years ago

I was thinking of any situation in which the button is reused, or the redirect happens in a new tab. Then your button would be permanently disabled on the original tab. Usually js buttons are implemented with a callback so that when the post response returns it re-enables the button. If you redirect to a new page it's fine, but if the implementation changes or gets reused it will also behave as expected. It was mostly a question for if any non-redirect pattern was expected. If so it should have an reenabled callback imo. If it's not a concern it's not blocking a merge.

mpacer commented 4 years ago

@mseal At least as of now I don't think we have any non-redirect pattern in the plans. I totally understand where you're coming from, and I think most js buttons don't have this fairly limited set of functionality.

But if you feel strongly we can reenable on error. I just wouldn't want to reenable on success since it will just be redirecting.

MSeal commented 4 years ago

Up to you, I was just trying to point out the potential issue. Feel free to merge whenever you're happy with changes.

mpacer commented 4 years ago

@MSeal I'll just merge for now and we can change it later if it turns out to be an issue. Thank you for raising it!