-
-
Notifications
You must be signed in to change notification settings - Fork 836
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
Replace spin.js with a CSS-only loading spinner #2764
Conversation
Is it just me, or does this spinner look different from the old one? Especially for the loading switch; that seems to block out the switch component completely? |
It does. There's no easy way to recreate the look of the spin.js spinner. What do you mean about blocking the switch? When the spinner shows, the switch should be disabled anyway -- changing it while it's loading causes state mismatch between the DB and frontend. |
I'm fine with the slight look change, and as for blocking the checkbox I'm also fine with that as it seems like good UI to me to prevent state mismatches and other issues. |
It doesn't actually block the switches, it just displays the spinner on top. You can still toggle it underneath unless protection is put into place in the JS to prevent it. |
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.
Perhaps I'm missing something, but where is the actual design of the spinner coming from?
Also, random idea, but could it be possible to use a fontawesome icon, and rotate that?
The design comes from the border. The indicator is a square, which gets given border radius 50% to effectively become a circle. We stick a border on all sides, except the top, of It's be possible to use FontAwesome, but I prefer this way as it's easier to customise in my opinion. |
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.
It's be possible to use FontAwesome, but I prefer this way as it's easier to customise in my opinion.
Could you elaborate on this? It feels like we're putting in a lot of code for this particular implementation that would need to be overriden completely given a different design
@askvortsov1 The main reason is the fact that there's only one loading-like FA icon we could use, which is If anyone wanted to change this for their theme, there would be no easy way to do so, except overriding CSS and changing the Using this method, anyone could just override the I also just really hate the look of |
94a1af3
to
457cf30
Compare
I much prefer the CSS implementation of the circle over anything font-awesome. |
Makes sense to me! Maybe we could add this info in a docblock in the component class? |
@askvortsov1 Done! :) |
Progresses flarum/issue-archive#179
Changes proposed in this pull request:
Reviewers should focus on:
This might be extension-breaking depending on how exts use the current loading spinner.
We might also need to make changes to how core exts use the Loading Spinner to make it render correctly. The main difference is how we set a custom height for the spinner container -- this needs to be done on a new class (
.LoadingSpinner-container
) instead of just.LoadingSpinner
. This is because of how the spinner is created using styles.Screenshot
Confirmed