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

Make it possible to mark messages important #2796

Merged
merged 2 commits into from
Apr 23, 2020
Merged

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Mar 26, 2020

@jancborchardt
Copy link
Member

As icon, you can use the same icon that Gmail uses: https://material.io/resources/icons/?search=label_important&icon=label_important&style=baseline

If a message is important, we can overlay it on the avatar just like the Favorite star.

@ChristophWurst
Copy link
Member Author

I can't find that icon in https://www.npmjs.com/package/vue-material-design-icons. What's its name?

@ChristophWurst

This comment has been minimized.

@jancborchardt
Copy link
Member

I can't find that icon in https://www.npmjs.com/package/vue-material-design-icons. What's its name?

It’s on the link / in the url: label_important. If that library doesn’t have it, just download it from the page and put it in the repository. :)

@ChristophWurst

This comment has been minimized.

@ChristophWurst
Copy link
Member Author

What do ya think

Bildschirmfoto von 2020-03-27 11-05-18

@jancborchardt
Copy link
Member

Looking good! Could use some more opacity, and ideally a slight white border around the icon to separate it from the avatar – same for the favorites icon.

But how do we do this so it works with the dark theme? Should I make it 2 icons each, white border for normal, dark border for dark theme?

@ChristophWurst
Copy link
Member Author

But how do we do this so it works with the dark theme? Should I make it 2 icons each, white border for normal, dark border for dark theme?

How does it work with the other icons? Don't we do this automagically?

@jancborchardt
Copy link
Member

But how do we do this so it works with the dark theme? Should I make it 2 icons each, white border for normal, dark border for dark theme?

How does it work with the other icons? Don't we do this automagically?

We currently do not do this visual separation for any icons which overlay other things. Cases where it’s similar:

  • Files: The favorites icon overlays as well, but has no visual separation. Would look nicer there too, so we could actually do this in the core favorites icon.
  • Notifications: On new notifications, the red dot on the bell icon is part of the icon and the bell has a cutout – it’s manually made as different icons, as the background of the header also needs to shine through.
  • Android: On Android we have a white border around the star icon, and need to adjust this now for dark mode.
  • Text: The avatars overlap and have a 2px border which visually separate them. We can do this here as they are HTML elements and not images.

cc @skjnldsv @juliushaertl is it possible to add this border programmatically, and adjust it based on the theme – maybe like the shadow and the black/white version? it should be a hard 1 or 2px border of the background color, just like the avatars in the Text app.

@skjnldsv
Copy link
Member

skjnldsv commented Apr 1, 2020

cc @skjnldsv @juliushaertl is it possible to add this border programmatically, and adjust it based on the theme – maybe like the shadow and the black/white version? it should be a hard 1 or 2px border of the background color, just like the avatars in the Text app.

you can fill the whole icon if you want, the border, don't think so.
Unless you use inline svg in the html code, then you can with css vars

@GretaD
Copy link
Contributor

GretaD commented Apr 20, 2020

Now it looks like this, after the style fixup.

white_imp_star
IMPOTAT-FGLAG

Not related to this but should we maybe change also the star icon and add a border, or remove the border from the important one, just to have the consistency.. cc @jancborchardt

@jancborchardt
Copy link
Member

Not related to this but should we maybe change also the star icon and add a border, or remove the border from the important one, just to have the consistency.. cc @jancborchardt

Yep, already talked to @skjnldsv that of course the star icon should have the same style with border. He mentioned the standardization of list items to be a blocker for that – correct me if that was wrong @skjnldsv. :)

@skjnldsv
Copy link
Member

He mentioned the standardization of list items to be a blocker for that – correct me if that was wrong @skjnldsv. :)

Yep, this is coming from server, until we have a nice dedicated component in #nextcloud-vue, there is no proper way to do it (inline svg + css variables)
@ma12-co had some components for the talk app, could we make them available everywhere on vue components & allow us to add inline svg around the avatar (like they're doing with the video icon & star?

@marcoambrosini
Copy link
Member

until we have a nice dedicated component in #nextcloud-vue, there is no proper way to do it (inline svg + css variables)

I hope I'll be able to give some love to this once the call view in talk is done

@ChristophWurst ChristophWurst force-pushed the feature/important-messages branch from c12e44f to 6f5890f Compare April 21, 2020 14:37
@ChristophWurst ChristophWurst marked this pull request as ready for review April 21, 2020 14:37
@ChristophWurst
Copy link
Member Author

@nextcloud/mail please give this a test. The basic marking as important should just work. The automatic classification is very basic. Let's get this implementation in and see how it performs. Then we can improve it.

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Apr 21, 2020

SQLSTATE[42883]: Undefined function: 7 ERROR:  operator does not exist: integer = character varying

LINE 1: ... INNER JOIN "oc_mail_mailboxes" "mb" ON "mb"."id" = "m"."mai...

                                                             ^

HINT:  No operator matches the given name and argument type(s). You might need to add explicit type casts.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

DB stuff looks good!

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the feature/important-messages branch from fce904a to 74d5c73 Compare April 23, 2020 07:52
@GretaD GretaD merged commit e5f0e35 into master Apr 23, 2020
@GretaD GretaD deleted the feature/important-messages branch April 23, 2020 08:59
@jancborchardt
Copy link
Member

O lala, we’re getting closer! :) Congrats everyone 🎉

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.

mail_messages.mailbox_id should be an INT
6 participants