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 Primary action icons #6568

Merged
merged 1 commit into from
May 30, 2022
Merged

Migrate Primary action icons #6568

merged 1 commit into from
May 30, 2022

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented May 27, 2022

@GretaD GretaD self-assigned this May 27, 2022
@GretaD GretaD requested review from st3iny and kesselb May 27, 2022 12:21
@GretaD GretaD changed the title Migrate MenuEnvelope icons Migrate Primary action icons May 27, 2022
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Looks good so far. I left some feedback and pushed a small commit to make the new important icon a bit more consistent with material icon components.

src/components/Envelope.vue Outdated Show resolved Hide resolved
src/components/Envelope.vue Outdated Show resolved Hide resolved
src/components/Envelope.vue Outdated Show resolved Hide resolved
src/components/Envelope.vue Outdated Show resolved Hide resolved
src/components/Envelope.vue Outdated Show resolved Hide resolved
src/components/MenuEnvelope.vue Outdated Show resolved Hide resolved
src/components/MenuEnvelope.vue Outdated Show resolved Hide resolved
src/components/MenuEnvelope.vue Outdated Show resolved Hide resolved
src/components/MenuEnvelope.vue Outdated Show resolved Hide resolved
src/components/MenuEnvelope.vue Outdated Show resolved Hide resolved
@GretaD GretaD added this to the v1.13.0 milestone May 27, 2022
@GretaD GretaD force-pushed the fix/migrate-icons-flags branch 2 times, most recently from 2406eb5 to bca5ff3 Compare May 30, 2022 07:16
@GretaD GretaD marked this pull request as ready for review May 30, 2022 07:19
@JuliaKirschenheuter
Copy link
Contributor

Thanks for fixing!

I saw a visual difference, the high between icon and text is a bit too large

before

image

after

image

@GretaD
Copy link
Contributor Author

GretaD commented May 30, 2022

Thanks for fixing!

I saw a visual difference, the high between icon and text is a bit too large

Fixed, also filled the gold colour for favorite.

@JuliaKirschenheuter
Copy link
Contributor

Fixed, also filled the gold colour for favorite.

works!

I think there is still too much padding around action buttons in primary menu

image

image

@ChristophWurst
Copy link
Member

We can fine tune after the design review

@GretaD
Copy link
Contributor Author

GretaD commented May 30, 2022

Fixed, also filled the gold colour for favorite.

works!

I think there is still too much padding around action buttons in primary menu

You are right, but the design material forces the icons to be always on the center. I just moved the text to be more up, i didnt redesign the way how the design icons looks.
See also here: #6500
the after for the mark all as read.

@GretaD GretaD requested a review from ChristophWurst May 30, 2022 11:17
@GretaD GretaD force-pushed the fix/migrate-icons-flags branch from b0e35b8 to fbf2f7f Compare May 30, 2022 12:56
@JuliaKirschenheuter JuliaKirschenheuter self-requested a review May 30, 2022 13:07
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

Greta, i have finally found the icon https://fonts.google.com/icons?selected=Material+Icons&icon.query=important =)


but it seems that there is not vue component for it, am i right?

@ChristophWurst
Copy link
Member

It is not in https://materialdesignicons.com/

@ChristophWurst
Copy link
Member

It is not in https://materialdesignicons.com/

This is the basis for https://www.npmjs.com/package/vue-material-design-icons. I'm not sure if we can use Google's icon if it's not in there.

@GretaD GretaD merged commit 46c77a0 into main May 30, 2022
@GretaD GretaD deleted the fix/migrate-icons-flags branch May 30, 2022 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants