Disable fork button while forking #1667
Conversation
Fixes codesandbox#844. This is a first fix. I'd like to work on a better feedback, maybe a circular loader inside the button. What do you think?
|
I think it would also be nice to have a positive feedback, like maybe changing the text to "Forking...". |
|
Totally agreed, I've just updated the PR. |
|
Hey!! I was also just working on this! Bless you!! There is also a couple more places we have that button, it would be awesome to have that there too 😁 |
|
@SaraVieira I've just checked and found this place where there is also a fork button: Do you see any other places? |
|
Awesome!! I'm at home and my laptop is in the car but the code looks great and I will check the UI Tomorrow morning and most likely merge it 😁 Can you add to the contributors table ? |
|
I'm already in the contributors table 🙂 |
|
What do you guys think about this animation?
|
|
I really like it! Nice css loading 🎉 |
There was a problem hiding this comment.
Man this is really clean! I left some really small nitpicks, but I love this implementation. Regarding the animation, I think it would be nicest to use something like the third from https://nzbin.github.io/three-dots/, we used three dots animations before for loading (not sure if we still have them). After those changes this is ready to merge!
| small | ||
| > | ||
| <> | ||
| <Fork style={{ marginRight: '.5rem' }} /> | ||
| Fork | ||
| {isForking ? 'Forking' : 'Fork'} |
There was a problem hiding this comment.
I think it would be best to name it "Forking..." with the three dots to indicate an action.
There was a problem hiding this comment.
Yes, i was hesitating between both because the dots makes the button really larger while forking. Now you convinced me :)
| disabled={isForkingSandbox} | ||
| onClick={this.forkSandbox} | ||
| > | ||
| {isForkingSandbox ? 'Forking Sandbox' : 'Fork Sandbox'} |
There was a problem hiding this comment.
Same here, adding 3 dots ... would be nice.
|
I'll keep the animation for a follow-up PR. I'll update following requested changes once I have access to my computer :) |
|
Also, maybe make the button a bit wider, so that when you change the text to "Forking" it doesn't do that distracting wiggle. |
|
I think we need to decide between both solutions, because having "Forking..." will force to have the Fork button pretty large. This will look weird compare to siblings buttons. |
|
For the animation, being fancy depends on whether we want to change the button markup or not. In my screenshot, I only used one before pseudo element. |
|
Just played with it, works really well! Thanks for this contribution @ValentinH! |
This is a follow-up of codesandbox#1667. The effect is simple because for now I didn't want to modify the markup of the button. As far as my limited CSS skills lead me, @CompuIves suggestion regarding nzbin.github.io/three-dots would require to have a div wrapper around the button and a div to display the 3 dots. Do we want to do this?
This is a follow-up of codesandbox#1667. The effect is simple because for now I didn't want to modify the markup of the button. As far as my limited CSS skills lead me, @CompuIves suggestion regarding nzbin.github.io/three-dots would require to have a div wrapper around the button and a div to display the 3 dots. Do we want to do this?
* Show a loader on the button while forking This is a follow-up of #1667. The effect is simple because for now I didn't want to modify the markup of the button. As far as my limited CSS skills lead me, @CompuIves suggestion regarding nzbin.github.io/three-dots would require to have a div wrapper around the button and a div to display the 3 dots. Do we want to do this? * Introduce ProgressButton to have more possibilities for the loader * Make the circles a bit smaller
Fixes #844.
This is a first improvement:
After this, I'd like to work on a better feedback, maybe a circular loader inside the button. What do you think?
Checklist: