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

Let apps toggle an unread counter on app icons #26939

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented May 11, 2021

image

The count value is currently hidden as we don't want to clutter the UI too much, but might be useful at some point.

It can either be updated during page load through the navigation manager $navigationManager->setUnreadCounter('activity', 10); or through JavaScript by calling OC.setNavigationCounter('deck', 123)

@@ -32,6 +32,20 @@ import OC from '../OC'
* If the screen is bigger, the main menu is not a toggle any more.
*/
export const setUp = () => {

Object.assign(OC, {
setNavigationCounter(id, counter) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit undecided on the approach to take here, maybe we should rather listen to an event that might be emitted then by the apps through the event bus?

Copy link
Member

Choose a reason for hiding this comment

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

I'd advocate against the global variable approach, at least form an app API perspective. We can hide this in yet another @nextcloud/xyz package or add it to an existing one. Then we can also later on change the way this action propagates from an app to the server component.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, then lets keep the OC.setNavigationCounter for use in the additional package then which we then can use for offering a stable API. Unfortunately I don't think we have any fitting package for that already so it would need an additional one.

@jancborchardt
Copy link
Member

For reference what I also said in the chat:
While the dot below looks very nice, it is most known as "active" indicator instead of for "notifications":

In the notifications app we provide 2 icons, 1 regular and 1 with dot. We could do the same with the app icons as well, and standardize the position and size via recommendation. That way we can also achieve the cut-out border for the dot which is difficult to do via CSS because we have a fade in the header.

@rullzer rullzer added this to the Nextcloud 22 milestone May 21, 2021
@MorrisJobke MorrisJobke mentioned this pull request May 26, 2021
98 tasks
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@blizzz
Copy link
Member

blizzz commented Jun 2, 2021

what's the state here? moving to 23?

@juliusknorr juliusknorr force-pushed the enh/app-icon-notification-bubble branch from 7db0abe to 8818011 Compare June 10, 2021 08:17
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 tasks
@juliusknorr juliusknorr force-pushed the enh/app-icon-notification-bubble branch from 8818011 to d563317 Compare June 10, 2021 08:38
@juliusknorr
Copy link
Member Author

Ready review, we'd still need this for 22.

@blizzz
Copy link
Member

blizzz commented Jun 10, 2021

CI is unhappy

core/src/components/MainMenu.js Outdated Show resolved Hide resolved
@juliusknorr juliusknorr force-pushed the enh/app-icon-notification-bubble branch 2 times, most recently from 489f341 to a73b6aa Compare June 15, 2021 20:18
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

👍 design-wise :)

@juliusknorr juliusknorr force-pushed the enh/app-icon-notification-bubble branch from a73b6aa to a0f081d Compare June 16, 2021 09:46
@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 16, 2021
@juliusknorr
Copy link
Member Author

Pushed another fix for the tests, waiting for CI to pass

@blizzz blizzz mentioned this pull request Jun 16, 2021
45 tasks
@blizzz
Copy link
Member

blizzz commented Jun 16, 2021

CI is unhappy

Co-authored-by: Louis Chemineau <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr force-pushed the enh/app-icon-notification-bubble branch from a0f081d to a942364 Compare June 17, 2021 06:16
@blizzz blizzz merged commit eec7924 into master Jun 17, 2021
@blizzz blizzz deleted the enh/app-icon-notification-bubble branch June 17, 2021 11:33
@blizzz blizzz mentioned this pull request Jun 23, 2021
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants