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

Improved UI of Switch with loading indicator #2039

Merged

Conversation

askvortsov1
Copy link
Sponsor Member

Fixes #1491

Changes proposed in this pull request:
Moved loading indicator outside of switch slider on settings page to improve ui

Reviewers should focus on:
Seems trivial.

Screenshot
https://i.imgur.com/6A8pGfZ.png
https://i.imgur.com/X4q7dnP.png

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@dsevillamartin
Copy link
Member

I don't think this is the fix we want(ed) to go with. Apart from content jumping around horizontally when the loading indicator appears - which was mentioned by Franz, I think Franz's suggestion is a better way to tackle this.

Since I would like to avoid page content jumping around (which might happen with the indicator appearing next to the toggle button), what about adding a semi-transparent overlay to the button while it is saving the state, with the indicator on top? That way, it would be more visible...
- #1491 (comment)

@askvortsov1
Copy link
Sponsor Member Author

Not 100% sure what you mean by 'jumping around horizontally': at least from my testing, the indicator doesn't displace anything, it just shows up. I'm not sure that making that switch (more) transparent would result in a more pleasant UI

@dsevillamartin
Copy link
Member

If the switch has items before it, it will not look well. I think what Franz was thinking of was having the loading indicator on top of the whole component with a semi-translucent background.

@askvortsov1
Copy link
Sponsor Member Author

Ah yeah that'd do it. So we could do Franz's idea with an overlay and the indicator in the middle. Alternatively, we could also move the indicator to the other side of the switch when it's going from on to off so that it's always on a white background.

@dsevillamartin
Copy link
Member

I think that'd get a bit confusing. Putting the loading over the whole thing is probably the best idea.

@askvortsov1
Copy link
Sponsor Member Author

Updated, how's this?

https://youtu.be/H2Yzu5U8574

@dsevillamartin
Copy link
Member

Still a bit hard to read IMO, the background might still be too transparent.

@askvortsov1 askvortsov1 changed the title Moved loading indicator outside of checkboxes to improve ui Improved UI of Switch with loading indicator Mar 12, 2020
@askvortsov1
Copy link
Sponsor Member Author

Just to confirm, are you proposing more or less transparency on the switch?

@dsevillamartin
Copy link
Member

Uh I'd say more transparent? The loading indicator isn't very visible right now.

@askvortsov1 askvortsov1 force-pushed the as/improve_checkbox_loading_indicator_ui branch from df35a1d to f43783f Compare March 20, 2020 14:40
@askvortsov1
Copy link
Sponsor Member Author

@datitisev how's this: https://youtu.be/hYxWwHgtd6k

@dsevillamartin
Copy link
Member

I think that's good? It may still be hard to view for some people, but I'd rather have other members of the team comment their thoughts before making you do more work.

@tankerkiller125
Copy link
Contributor

I'm happy with the latest revision, it looks much better, the only thing I could possible think to improve is make the actual loading wheel just a tad darker maybe?

@askvortsov1
Copy link
Sponsor Member Author

How's this: https://youtu.be/4JJUz0MC6YQ? Too dark?

@dsevillamartin
Copy link
Member

@askvortsov1 Looks fine to me. How does it look in dark mode?

image

@askvortsov1
Copy link
Sponsor Member Author

Good catch, thanks! How's this: https://youtu.be/xeuNbixI-sU?

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the latest changes

Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@franzliedke franzliedke merged commit ffa5659 into flarum:master Apr 10, 2020
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

Successfully merging this pull request may close these issues.

Loading state of Switch is hard to see
4 participants