-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
PR: Add API to start/stop animations to the Spin class #245
Conversation
Should we mark this as the fix for #165 @ccordoba12 ? |
Yep, you're totally right (I didn't know about that issue). |
And my question for you: should we add documentation for this new feature? And if so, where should I put it? |
75928a4
to
ea8c2c0
Compare
That's a good point, I would say yes 👍
Probably we could expand the Lines 158 to 166 in d1371a7
qtawesome/docs/source/usage.rst Lines 171 to 179 in d1371a7
Lines 123 to 127 in d1371a7
Maybe in the future we should try to expand the API reference to cover not only the |
This also adds a kwarg to decide if the animation should be automatically started (in some situations that's not desired).
ea8c2c0
to
80bd2de
Compare
Done in the three places you referenced above. |
Note: Seems like on Windows with PyQt5 running the |
Fixed in my last commit by skip running the example file when using PyQt5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ccordoba12 ! Regarding your last comment:
Fixed in my last commit by skip running the example file when using PyQt5.
I think is fine for the moment but probably worthy to understand why the running the example.py
run is failing over CI, no? Should we at least create a follow up issue for that before merging this as it is? What do you think?
Yeah, that's a good point. I think the problem has to do with the new timers I added to start/stop animations of some icons.
Agreed. I'll create one and take a look at it after Spyder 6 alpha3 is released. |
Created #254 to proceed with the merge here |
Fixes #165.