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

Notification screen & tray icons customizations #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

surinder-tsys
Copy link

@surinder-tsys surinder-tsys commented Apr 13, 2023

NMC-1902: Notification customizations.
NMC-2250: Notification tray icon customized.

@surinder-tsys surinder-tsys added the custom MagentaCLOUD customisation label Apr 13, 2023
@tobiasKaminsky
Copy link

2023-04-13-151527 2023-04-13-151117
(nc vs mc)

@surinder-tsys
Copy link
Author

surinder-tsys commented Apr 13, 2023

@tobiasKaminsky
This PR contains the following:

  1. Divider added between the list items.
  2. Action buttons are customized when someone share's the file. Refer the below screenshot:
    Screenshot_20230413_185132

@tobiasKaminsky
Copy link

image

So change is that secondary button is not filled?

@tobiasKaminsky
Copy link

https://m3.material.io/components/buttons/overview

image

We can use this to update our buttons…
@jancborchardt which do you prefer or shall we keep (2) for primary and (3) for secondary?
Or go with (2) for primary and (4) for secondary?

@jancborchardt
Copy link

Cause there can be a lot of primary and secondary buttons here, we should go with (3) tonal for primary and (5) text for secondary. (2) filled should only be used for primary if it is the only primary on the screen.

Also 2 remarks:

  • We can take the divider lines upstream too, no need to only put that in a fork?
  • The "Accept"/"Reject" buttons don't really make sense for any of the notifications shown in the screenshot of Notification screen & tray icons customizations #66 (comment) – in all of these cases it only needs a call to action button, and the "Reject" button is the dismiss x on the top right.

@tobiasKaminsky
Copy link

Cause there can be a lot of primary and secondary buttons

No, this is not possible. We agreed to have only 1 primary (obviously) and at most 1 secondary. If more, then this will be "more" button, leading to a list.

@tobiasKaminsky
Copy link

We can take the divider lines upstream too, no need to only put that in a fork?

divider were removed upon your request back then. We do not have them anywhere, not in files, not in activity.

  • The "Accept"/"Reject" buttons don't really make sense for any of the notifications shown in the screenshot of

I assume that this is only testing content.

@tobiasKaminsky
Copy link

Cause there can be a lot of primary and secondary buttons

No, this is not possible. We agreed to have only 1 primary (obviously) and at most 1 secondary. If more, then this will be "more" button, leading to a list.

Ah, no I probably understand it. You mean multiple primary on screen, not per notification 👍

Then we do it: (3) tonal for primary and (5) text for secondary.

@tobiasKaminsky
Copy link

If Jens on MC side is also fine with it, we can push it upstream.
No need to have it here.

@jancborchardt
Copy link

jancborchardt commented Apr 17, 2023

divider were removed upon your request back then. We do not have them anywhere, not in files, not in activity.

@tobiasKaminsky yep, and I would still say in the file list that is better as there’s not that much content. However notifications can get longer, so a divider can make sense there.
And above that, let’s just make sure the differences are minimal or nonexistent / mainly consist of theming. :) Makes all our lives easier.

@tobiasKaminsky
Copy link

oh boy…Jens wrote me in chat that it is fine to not have divider last week…
now we have two designers with two different opinions.

Please sort this out :) and then we follow :)

@surinder-tsys
Copy link
Author

@tobiasKaminsky Jens wrote me the same that no divider is required.
So I have updated the PR without divider.
Let me know if any other changes are required.

@surinder-tsys
Copy link
Author

image

So change is that secondary button is not filled?

@tobiasKaminsky that's right .
We are using secondary button as (4) outlined one and the color we used is not the primary color.

@tobiasKaminsky
Copy link

@surinder-tsys

  • can you change it to: (3) tonal for primary and (5) text for secondary.
  • move to upstream

Jan and Jens are fine with it.

@surinder-tsys
Copy link
Author

@surinder-tsys

  • can you change it to: (3) tonal for primary and (5) text for secondary.
  • move to upstream

Jan and Jens are fine with it.

@tobiasKaminsky I had conversation with Jens and he told that we will keep our style which is:

  1. Filled for Primary and Outlined for Secondary.
  2. Keeping the blue color for buttons
  3. Alignment of them on left side.

Please let me know if still this changes needed to be up-streamed or not required.

@jweidemann
Copy link

jweidemann commented Apr 21, 2023

@jancborchardt Can we please use 2 (filled) as primary? To accept or reject a share is really an action the user should do and normally it wouldnt stack to multople primary buttons in one screen. All the other notification usually don't have buttons there...

As Telekom we also have just 2 and 4 in use - so we would prefer outline button for reject. But if you insist for having 5 (text only button) we can live with that.

@surinder-tsys
Copy link
Author

Hi @jancborchardt can you please provide input for Jens previous comment?

@jancborchardt
Copy link

Can we please use 2 (filled) as primary?
As Telekom we also have just 2 and 4 in use - so we would prefer outline button for reject. But if you insist for having 5 (text only button) we can live with that.

@jweidemann @surinder-tsys excuse the late reply. Yes, 2 as primary is good with us as well! We never use outlined buttons, so for the reject button I would prefer 5 (text only). Also fine is 3 (lighter filled).

@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch 2 times, most recently from 63d6f0e to 778a105 Compare May 17, 2023 13:32
@surinder-tsys
Copy link
Author

Can we please use 2 (filled) as primary?
As Telekom we also have just 2 and 4 in use - so we would prefer outline button for reject. But if you insist for having 5 (text only button) we can live with that.

@jweidemann @surinder-tsys excuse the late reply. Yes, 2 as primary is good with us as well! We never use outlined buttons, so for the reject button I would prefer 5 (text only). Also fine is 3 (lighter filled).

@jancborchardt We are also ok with 5 (text only) for reject button. Will be updating the PR.

@surinder-tsys
Copy link
Author

@tobiasKaminsky PR has been up-streamed: nextcloud#11693

Upstreamed code will be removed once the PR is merged.

@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch 2 times, most recently from de90333 to 768bb08 Compare June 9, 2023 18:02
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch from 768bb08 to 95917e9 Compare July 6, 2023 08:40
@surinder-tsys surinder-tsys added the build-ready Customization to include into build label Jul 13, 2023
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch from 95917e9 to c87cb88 Compare August 28, 2023 19:24
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch 2 times, most recently from 0df6ffa to 80c33c5 Compare November 21, 2023 11:26
@surinder-tsys surinder-tsys changed the title Nmc/1902 notification Nmc/1902 Notification Feb 8, 2024
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch from 288dfb5 to f61a3a7 Compare April 8, 2024 18:54
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch from f61a3a7 to 7a35bf8 Compare August 5, 2024 06:51
@surinder-tsys surinder-tsys force-pushed the nmc/1902-notification branch from 7a35bf8 to db6aec3 Compare August 23, 2024 06:08
@surinder-tsys surinder-tsys changed the title Nmc/1902 Notification Notification screen & tray icons customizations Sep 3, 2024
NMC-2250: Notification tray icon customized
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved build-ready Customization to include into build custom MagentaCLOUD customisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants