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

Loadable slider #139

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Loadable slider #139

wants to merge 8 commits into from

Conversation

tinolaomahei
Copy link
Contributor

@tinolaomahei tinolaomahei commented Sep 8, 2020

Description

When performing an action using this widget, I think it would be useful to convey a loading state before the slider is complete or reset. This PR attempts to allow consuming apps to perform some work i.e. call an API, set the slider into a loading state and then once the work is complete, have the slider continue its complete animation.

Loading slider example

loading gif

Event logs with loading callback

event_log gif

Related PRs/Issues

#116

Todos

  • Loading animation

Please treat this PR as a prompt to a discussion rather than a mergeable PR as I'd imagine we'd want the loading icon to act like a spinner or perhaps allow the animation to be defined in an attribute on the slider?

Look forward to a review when you get the chance.

@cortinico
Copy link
Owner

Please treat this PR as a prompt to a discussion rather than a mergeable PR as I'd imagine we'd want the loading icon to act like a spinner or perhaps allow the animation to be defined in an attribute on the slider?
Look forward to a review when you get the chance.

First of all, thank you very much for your contribution 🙏 Also for providing screenshots and documentation, really appreciated.

I gave a first look at the code and it looks good. There are however a couple of discussion points we should solve:

  • I think this can actually live into its own widget LoadableSlideToActView? This will help separate the logic of the two animations
  • The widget is currently stateless. Before this PR can be merged, State management should be added.

@tinolaomahei
Copy link
Contributor Author

I gave a first look at the code and it looks good. There are however a couple of discussion points we should solve:

  • I think this can actually live into its own widget LoadableSlideToActView? This will help separate the logic of the two animations
  • The widget is currently stateless. Before this PR can be merged, State management should be added.

Thanks for taking a look at my PR!

Will look into separating out the loadable code to its own LoadableSlideToActView widget, I think that's a great idea actually. I'll also implement the loading animation in this new widget and look into adding state management. Did you have a certain state management design/architecture in mind or are you happy with me sending you an update to this PR including proper state management for you to review?

@cortinico
Copy link
Owner

cortinico commented Sep 22, 2020

Did you have a certain state management design/architecture in mind or are you happy with me sending you an update to this PR including proper state management for you to review?

Nothing specifically at the moment. Eventually a solution that relies on getDrawableState + some custom states could be interesting. Otherwise anything that is able to retain the state of the widget on configuration change would also work 👍

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

Successfully merging this pull request may close these issues.

2 participants