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

Replace "Saved" indicator with an icon #3572

Merged
merged 5 commits into from
Dec 16, 2022
Merged

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Dec 14, 2022

📝 Summary

Replaces the saving indicator with an icon, like the "active" users next to it. This also unifies the look on desktop and mobile devices.

🖼️ Screenshots

🏚️ Before 🏡 After
desktop saving
desktop saved
new saving
new saved
mobile
mobile saving
mobile saved
same as above

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@susnux susnux added enhancement New feature or request design Experience, interaction, interface, … 3. to review labels Dec 14, 2022
@susnux
Copy link
Contributor Author

susnux commented Dec 14, 2022

/compile

@cypress
Copy link

cypress bot commented Dec 14, 2022



Test summary

115 0 0 0Flakiness 1


Run details

Project Text
Status Passed
Commit 69fdf6d
Started Dec 16, 2022 2:06 PM
Ended Dec 16, 2022 2:11 PM
Duration 05:02 💡
OS Linux Ubuntu -
Browser Electron 106

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/nodes/Links.spec.js Flakiness
1 test link marks > link preview > shows a link preview

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@max-nextcloud max-nextcloud force-pushed the enh/saving-indicator branch 2 times, most recently from 75043a0 to 06fcf14 Compare December 14, 2022 05:56
{{ lastSavedStatus }}
<div v-tooltip="lastSavedStatusTooltip" class="save-status" :class="saveStatusClass">
<CheckIcon v-if="saveStatusClass === 'saved'" />
<Loading v-else-if="saveStatusClass === 'saving'" />

Choose a reason for hiding this comment

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

We have a loading icon in the vue-components. It resembles the style of the legacy loading icon used all over Nextcloud. Might be good to use it here, to keep it uniform.

See https://nextcloud-vue-components.netlify.app/#/Components/NcLoadingIcon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should then replace also the other usages of the loading icon within text with the icon from nextcloud-vue :)
But probably better on a follow up PR.

@nimishavijay
Copy link
Member

This is a great improvement! One suggestion: we can remove the background and make it look visually similar to the formatting options.

I'm also thinking instead of the loading indicator we can use the floppy disk symbol, making it look the same as the save indicator on Office.

@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

This is a great improvement! One suggestion: we can remove the background and make it look visually similar to the formatting options.

I added it to make it look similar to the active user list right to it, but maybe we should remove the background there too?

I'm also thinking instead of the loading indicator we can use the floppy disk symbol, making it look the same as the save indicator on Office.

Used the check icon following this comment by @jancborchardt #2734 (comment)

@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

I pushed a commit to unify the look of the menu bar with the status & active user indicators:

  • Set same size as menu bar entries: 44px
  • Align icons vertically with menu bar
  • Remove background and add background when hovering like menu bar

It looks now like this:

voko.mp4

And BTW on error it would look like this:
saving indicator showing red warn sign

@raimund-schluessler
Copy link
Member

Would be nice to align the saving indicator with the three-dots-button above.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Code looks good to me. From my point of view this is quite a big improvement.
I'd say this can be merged as is. Aligning the icons would be nice but i don't think it should block this. Using NcLoadingIcon everywhere was already postponed to a follow up PR.

@mejo-
Copy link
Member

mejo- commented Dec 15, 2022

Looks great, thanks a lot @susnux. Given that sometimes it takes quite some time until the status is saved, I wonder whether the current rotating loading spinner is a bit too annoying for something that should be safe to ignore in most cases. What do you think=

@mejo-
Copy link
Member

mejo- commented Dec 15, 2022

But I'm also fine with discussing this in a follow-up issue/PR.

@max-nextcloud
Copy link
Collaborator

Looks great, thanks a lot @susnux. Given that sometimes it takes quite some time until the status is saved, I wonder whether the current rotating loading spinner is a bit too annoying for something that should be safe to ignore in most cases. What do you think=

Actually this is a good point. I think right now we only save every now and then to not overwhelm the server. So actually there are three states:

  • saved
  • modified but not saving yet
  • saving right now

I think modified is currently treated as saving and shows the spinning wheel. Maybe we could show a pencil instead. Since that requires some additional logic i'd also opt for a follow up issue / pr.

@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

@mejo- Good point!
And after reading the comment by @jancborchardt mentioned above again, he already proposed using the dot indicator for unsaved changes.

Fixed this with the latest commit, it now looks like this:

vokoscreenNG-2022-12-15_15-20-03.mp4

@nimishavijay
Copy link
Member

I think modified is currently treated as saving and shows the spinning wheel. Maybe we could show a pencil instead.

Suggestion: this icon used in Office
image
I can't find the MDI icon for this as it seems to be a custom icon, but it was suggested in the same comment by Jan that @susnux linked. We are trying to move towards MDI icons but this is used in Office and seems quite fitting as well :)
cc @jancborchardt

@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

Would be nice to align the saving indicator with the three-dots-button above.

@raimund-schluessler While this is possible, it might be better to exchange the positions of the current users and the saving indicator and align the saving with the close and the current users with the ... menu.
Because the current user button expands to the left if an user joins the session, so the alignment would be useless:

current user menu expanded to the left so alignment has no use

(Nevertheless I aligned both button with the upper ones in the latest commit)

@susnux
Copy link
Contributor Author

susnux commented Dec 15, 2022

@nimishavijay Ok I think I misunderstood the comment, I thought we should use a check mark for saved and a dot for unsaved. But it should look like this (?):

vokoscreenNG-2022-12-15_16-44-59.mp4

Copy link
Member

@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.

That is perfect, super nice! :) 🚀

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
* Set same size as menu bar entries: 44px
* Align icons vertically with menu bar
* Remove background and add background when hovering like menu bar

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Dec 16, 2022

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review design Experience, interaction, interface, … enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants