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

Migrate icons to vue material design icons #2439

Merged
merged 67 commits into from
May 31, 2022

Conversation

vinicius73
Copy link
Member

@vinicius73 vinicius73 commented May 23, 2022

Summary

Changes MenuBar implementation to use Material Design Icons.

The previous implementation of MenuBar was very bounded with icon classes.
To migrate to MDI we have changed the implementation to a modern and performant approach.

  • Use MDI in MenuBar entries
  • Change Table Actions Icons
  • Change callouts icons
  • Remove old icons
  • Fix tests

image

image

image

image

image

image

image

image

image

image

image

Vinicius Reis added 14 commits May 13, 2022 15:58
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
improves performance and responsivity
isolates responsability of image upload

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
improve style/ux

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@vinicius73 vinicius73 added enhancement New feature or request design Experience, interaction, interface, … 2. developing performance 🚀 labels May 23, 2022
Vinicius Reis added 9 commits May 23, 2022 17:41
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Vinicius Reis added 2 commits May 27, 2022 13:32
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
…hub.com:nextcloud/text into 2345-migrate-icons-to-vue-material-design-icons
<EmojiPicker class="entry-action entry-action__emoji"
:data-text-action-entry="actionEntry.key"
@selectData="addEmoji">
<button v-tooltip="actionEntry.label"
Copy link
Member

@juliusknorr juliusknorr May 27, 2022

Choose a reason for hiding this comment

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

We could switch to the @nextcloud/vue button component here in order to avoid custom styling from https://github.com/nextcloud/text/pull/2439/files#diff-ad1f213fdd71dfbb7fd8df37ade6a17b7c16e70098218a4b76cce502ee857eabR18

That way the styles should also be the same as with the other icons in regards to opacity and focus style:

Screenshot 2022-05-27 at 18 42 05

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks awesome 🚀

Just a small comment in regards to reusing a shared component instead of styling our own button, but not a blocker from my side.

@juliusknorr
Copy link
Member

One other small thing, some buttons (the Actions with a submenu) seem to have a slightly different focus-visible style:

Screenshot 2022-05-27 at 18 41 40
Screenshot 2022-05-27 at 18 41 43

But maybe something to report in server/nextcloud-vue

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Very nice. I'll do a deeper review soon.

src/components/EditorMidiaHandler.vue Outdated Show resolved Hide resolved
src/components/EditorMidiaHandler.vue Outdated Show resolved Hide resolved
Vinicius Reis added 2 commits May 30, 2022 10:16
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@julien-nc
Copy link
Member

No more comments, impressive changes.
Ready to merge after the small requested changes.

Vinicius Reis added 2 commits May 30, 2022 12:44
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@vinicius73
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
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.

Really really nice! Amazing work :)
I only have 2 tiny changes:

  • The icons for "Info" and "Warning" look very similar so I would suggest using the triangle icon alert instead
  • For "Add link" we could use the icon link as I think it is more recognisable for adding links. I do remember there being a discussion about the icons for "Add link" and "Attachment" being too similar, though. @jancborchardt what do you think?

Other those 2 points it is super cool!

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@vinicius73
Copy link
Member Author

image

image

image

image

image

I've changed the icons @nimishavijay
What do you think?

Vinicius Reis added 4 commits May 30, 2022 15:53
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
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.

Looks great now! 🚀

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

👍

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@vinicius73
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@vinicius73 vinicius73 merged commit 5120dae into master May 31, 2022
@delete-merged-branch delete-merged-branch bot deleted the 2345-migrate-icons-to-vue-material-design-icons branch May 31, 2022 13:49
@susnux susnux mentioned this pull request Jun 17, 2022
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 performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate icons to vue-material-design-icons
5 participants