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

Provide feedback when user refreshes account #451

Merged
merged 5 commits into from Dec 14, 2017

Conversation

Projects
None yet
2 participants
@Nasicus
Contributor

Nasicus commented Dec 11, 2017

I noticed that when you click the refresh button no progress and no feedback is provided.

I added an animation to the refresh button when a refresh is being done (which most often you can't even see because it's too fast - but you never know.. ;) ) and added feedback (toast) when the refresh is done.

The whole thing looks like this (note: I added a manually timeout of 3000ms for demo purposes):

random

I also disabled "button-spamming" which basically means when a request is already executing clicking the button again will not do another request.

I have 3 questions:

  • What do you think in general? ;)
  • In the error case should the user get a specific error message or is my implementation enough?
  • Concering translations: Is it correct that I just pass them as a normal string to the toast service or do I need to do something more?

Greetings :)

@j-a-m-l j-a-m-l self-requested a review Dec 12, 2017

@j-a-m-l
  1. I like it and the animation is a good idea.
  2. Probably is enough.
  3. The toastService translates the texts.

Some changes:

  • When the user has the "Refresh accounts automatically" setting on, showing the toast every n seconds is very annoying. When that option is disabled and the user clicks on the refresh button is the expected behaviour.
  • Use the animation and the same behaviour on the dashboard refresh button too.
@Nasicus

This comment has been minimized.

Show comment
Hide comment
@Nasicus

Nasicus Dec 13, 2017

Contributor

So.....

When the user has the "Refresh accounts automatically" setting on, showing the toast every n seconds is very annoying. When that option is disabled and the user clicks on the refresh button is the expected behaviour.

=> fair enough, did not see that => fixed

Use the animation and the same behaviour on the dashboard refresh button too.

=> done, note that I had to fix a bug which always occurs for me (and prevented me to even test my refresh logic), I did the easiest possible fix, not sure if there need to be done more (however maybe not in this PR since it's not related to it and think my fix is also correct)

After I added the animation and logic to the second button I realized that there was a lot of code and logic duplication, so I decided to refactor it. By doing that I also made the logic way more generic - I'm quite happy with the result :) I hope you are too :D

One last thing:
Since I now use a helper "class" / object for the logic and showing the toasts:
Will the strings still be recognized as "translatables" or do I need them to register in a different way?

Contributor

Nasicus commented Dec 13, 2017

So.....

When the user has the "Refresh accounts automatically" setting on, showing the toast every n seconds is very annoying. When that option is disabled and the user clicks on the refresh button is the expected behaviour.

=> fair enough, did not see that => fixed

Use the animation and the same behaviour on the dashboard refresh button too.

=> done, note that I had to fix a bug which always occurs for me (and prevented me to even test my refresh logic), I did the easiest possible fix, not sure if there need to be done more (however maybe not in this PR since it's not related to it and think my fix is also correct)

After I added the animation and logic to the second button I realized that there was a lot of code and logic duplication, so I decided to refactor it. By doing that I also made the logic way more generic - I'm quite happy with the result :) I hope you are too :D

One last thing:
Since I now use a helper "class" / object for the logic and showing the toasts:
Will the strings still be recognized as "translatables" or do I need them to register in a different way?

@j-a-m-l j-a-m-l merged commit 215351c into ArkEcosystem:master Dec 14, 2017

@j-a-m-l

This comment has been minimized.

Show comment
Hide comment
@j-a-m-l

j-a-m-l Dec 14, 2017

Member

+5

Yes, it's enough that way: the toastService would translate the text as expected.

Member

j-a-m-l commented Dec 14, 2017

+5

Yes, it's enough that way: the toastService would translate the text as expected.

@Nasicus Nasicus deleted the Nasicus:feat/refresh-feedback branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment