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

Continuous spinner component not started on initialization #20

Open
albe opened this issue Mar 21, 2019 · 6 comments
Open

Continuous spinner component not started on initialization #20

albe opened this issue Mar 21, 2019 · 6 comments

Comments

@albe
Copy link

albe commented Mar 21, 2019

When a continuous spinner is initialized, it is not started/looped. Since a continuous spinner is supposed to loop indefinitely and there exists no stop, it should be started automatically.

Also, the spinner is not automatically initialized via upgradeAllElements() for no apparent reason (?).

@christophstockinger
Copy link

No, I don't think that the spinner should start immediately after initialization, because it can be initialized, but is not visible to the user. It would make more sense to start the spinner when you need it.
The initialization can take place for example when loading the page and the spinner should only be started when the user has performed an action.

And yes, there is no stop function. But you can solve this by simply hiding the spinner via CSS or use JavaScript to remove it from the DOM

To start the loop you can simply call the loop method.

// on page load
const spinners = Spinner. upgradeAllElements();

// after an action
spinners.forEach(spinner => {
spinner.loop();
});

@albe
Copy link
Author

albe commented Apr 2, 2019

Hey there, thanks for the response!

And yes, there is no stop function. But you can solve this by simply hiding the spinner via CSS or use JavaScript to remove it from the DOM

✔️ That's expected and perfectly fine.

the spinner should start immediately after initialization, because it can be initialized, but is not visible to the user.

True, but specifically for a continuous spinner, it makes no difference if it is already looping invisibly. The user can easily control visibility of the spinner with his own known methods as mentioned above. The main point though is API UX and making the usage of the component easier.

The concrete case that lead to the issue was:

  • a fully functional VUE App with upgradeAllElements() in place
  • we had to just add a spinner when waiting for an async API response
  • we added the spinner component HTML snippet, added visibility via CSS class and VUE toggle flag at the correct places
  • the spinner was not rendered, though all other components worked, which was unexepected
  • quite a bit of debugging time (in relation to time spent adding the spinner) was spent until we found we need to manually call Spinner.upgradeAllElements()
  • then the spinner was displayed, but it did not spin - again this was unexpected
  • another round of debugging/search through the ui kit code until we found, we need to manually trigger the loop() method on every spinner initialized

You see, there were two pain points that seem unnecessary in the specific case of continuous spinners. Would be cool if that could be remedied by either changing the behaviour or at least documenting the expected usage of the spinner component better. Thanks for your time!

@christophstockinger
Copy link

christophstockinger commented Apr 4, 2019

Well, that spinner covers several cases. It also shows a load spinner for upload or a continuous spinner.

But the documentary is still expandable. But I think Audi and Strichpunkt Design are going to continue using it.

When using Audi UI components I would be careful because of the missing license.

But if you have an excerpt of the code snippet I might be able to help you with how to install it. (Of course with the background that you start the Continuous Spinner with loop(). You can find the upgradeAll()-function here)

@albe
Copy link
Author

albe commented Apr 6, 2019

When using Audi UI components I would be careful because of the missing license.

Working on an Audi project as external contractor, so not an issue.

But if you have an excerpt of the code snippet I might be able to help you with how to install it.

Thanks for the offer. We got it all working, I just wanted to share the suboptimal experience to maybe improve the developer experience a bit.

@christophstockinger
Copy link

Okay, can you close the issue?

@albe
Copy link
Author

albe commented Apr 10, 2019

I still think there is someting to improve in the spinner component, so I leave it up to you to close the issue and/or create a new issue that is more actionable for you.

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

No branches or pull requests

2 participants