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

Add material design loading icon #2727

Merged
merged 4 commits into from
Jun 15, 2022
Merged

Add material design loading icon #2727

merged 4 commits into from
Jun 15, 2022

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Jun 3, 2022

This adds a component to show a loading icon with a material design icon.

Bildschirmaufzeichnung vom 14 06 2022, 21 23 09

@raimund-schluessler raimund-schluessler added this to the 6.0.0 milestone Jun 3, 2022
@raimund-schluessler raimund-schluessler self-assigned this Jun 9, 2022
@raimund-schluessler raimund-schluessler changed the title Add component to show material design loading icon Add material design loading icon Jun 14, 2022
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler marked this pull request as ready for review June 14, 2022 19:24
@raimund-schluessler raimund-schluessler added enhancement New feature or request 3. to review Waiting for reviews design Design, UX, interface and interaction design labels Jun 14, 2022
@skjnldsv
Copy link
Contributor

Please also add a way to override the colour :)
Example, loading an image for Viewer in an always-dark background

@tcitworld
Copy link
Contributor

Maybe either pass all props or declare all props from vue-mdi (title, fillColor) ?

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Jun 15, 2022

Maybe either pass all props or declare all props from vue-mdi (title, fillColor) ?

Done. I just don't have npm at hand right now, so if someone could fix the l10n extract, that would be great 🙈😄.

loading

Signed-off-by: greta <gretadoci@gmail.com>
Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

I don't have any strong opinions on changing the colour of the spinner, but the default colour of the spinner should the same as the text in the button. If that's how it is right now, then 🚀🚀🚀

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

I don't have any strong opinions on changing the colour of the spinner, but the default colour of the spinner should the same as the text in the button. If that's how it is right now, then 🚀🚀🚀

Not sure, what you mean. There is no button in the component 🤔 The default color used now is --color-loading-dark, because I thought that would be fitting for a loading icon.

The spinning part of the old loading-icon had the color #444, which is the same as the now used --color-loading-dark. So I guess that's fine?

@skjnldsv
Copy link
Contributor

The spinning part of the old loading-icon had the color #444, which is the same as the now used --color-loading-dark. So I guess that's fine?

Let's keep using that one please yes.
If we need to change it, that should be from the server variables

@skjnldsv skjnldsv merged commit e1e3486 into master Jun 15, 2022
@skjnldsv skjnldsv deleted the feature/noid/loading-icon branch June 15, 2022 08:58
@jancborchardt
Copy link
Contributor

But developers should not be able to pick any color for the spinner – it should just be light or dark, depending on the background?

@skjnldsv
Copy link
Contributor

But developers should not be able to pick any color for the spinner – it should just be light or dark, depending on the background?

Fair enough, a color prop might be too strong, maybe a force-dark or force-white prop that limit the colours being used?

@jancborchardt
Copy link
Contributor

jancborchardt commented Jun 16, 2022

@skjnldsv that would not work with light and dark theme though, right? We need

  1. a default color which works with the color-main-background (as well as in inputs and tertiary buttons) depending on light/dark theme
  2. another one which works on the primary color (e.g. in header or buttons), depending on server theming
  3. possibly another one in primary color which works in these new buttons we use some places which are color-primary-light. (Worst case we just use 1) for those too.)
  4. EDIT: Forgot about the case where it’s a viewer background which could be dark or light, depending on app as well (dark for Photos, light for Text and Office at least in light theme).

This could possibly be automated depending on the parent, e.g. it would be 1) most often, but if it’s in a primary button it would automatically be 2).

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Jun 16, 2022

But developers should not be able to pick any color for the spinner – it should just be light or dark, depending on the background?

Fair enough, a color prop might be too strong, maybe a force-dark or force-white prop that limit the colours being used?

How about a prop style (or similar name) with the options dark, light and auto (being default).
And which colors (or variables) should we use for dark and light (for auto it will stay var(--color-loading-dark), I guess).

Edit: @jancborchardt Sorry, didn't see your comment before submitting mine.

@skjnldsv
Copy link
Contributor

How about a prop style (or similar name) with the options dark, light and auto (being default).

Yes, perfect!

@jancborchardt for 4, this what we're discussing.
You need to be able to force light or dark. See previous comments.

another one which works on the primary color (e.g. in header or buttons), depending on server theming
possibly another one in primary color which works in these new buttons we use some places which are color-primary-light. (Worst case we just use 1) for those too.)

We do not use the loading primary anywhere at nextcloud. Please provide clear guidelines on why and when to use them in an issue so we can implement this if it's really necessary 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UX, interface and interaction design enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants