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

Due date notification setting per board #2430

Merged
merged 15 commits into from
Nov 9, 2020
Merged

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 12, 2020

Summary

  • Implement a notification setting per board that allows to adjust if due date notifications should be sent
    • for all cards
    • only for assigned cards default
    • never

The user interface was slightly adjusted since having all those settings in the main menu feels to clutter a lot already, so in contrast to Talk where the options are shown directly this is now hidden in a submenu:

image

image

TODO

  • Proper icons instead of radio buttons
  • Update API documentation for new config endpoints and extended board result
  • Check with @jancborchardt if we should make "all cards" the default for boards that are not shared yet
  • Cover assignement notification handling with tests

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

See comments

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Code looks clean

@juliusknorr juliusknorr force-pushed the enh/due-notify-settings branch from 0ad609d to 1309002 Compare October 14, 2020 15:28
@juliusknorr juliusknorr force-pushed the enh/due-notify-settings branch from 1309002 to 07b8b3b Compare November 3, 2020 13:48
@juliusknorr
Copy link
Member Author

Quick update on the icons to align with talk:

image

@jancborchardt
Copy link
Member

jancborchardt commented Nov 5, 2020

Very nice with the submenu! :) Is that a component already which could be used elsewhere?

Generally it’s super nice already, here’s some possibility for enhancement, your call if or when it gets done. :D

  • For proper feedback: A subline for the "Due date reminders" entry with the chosen setting (like you know from Android and iOS) because otherwise you have to go into the submenu to know the active setting. The icon could also reflect the chosen setting.
  • Nice to have: A quick animation left/right when the submenu gets shown and when you go back.
  • This is more generally about the ActionMenu component (cause this is also the case for Talk), but the hover/focus and active style needs to be adjusted to move from the blue bar & text opacity towards just changing the background. cc @ma12-co

@jancborchardt
Copy link
Member

Check with @jancborchardt if we should make "all cards" the default for boards that are not shared yet

Yeah, good question. Cause then would it switch to "Assigned cards" when you share it?

@juliusknorr
Copy link
Member Author

Yeah, good question. Cause then would it switch to "Assigned cards" when you share it?

Yes, we could of course just hide the assigned setting if a board is not shared and just offer on/off then. Of course this will need some careful handling of adjusting the setting if all shares get removed from a board.

@jancborchardt
Copy link
Member

Yes, we could of course just hide the assigned setting if a board is not shared and just offer on/off then. Of course this will need some careful handling of adjusting the setting if all shares get removed from a board.

Right, then for a non-shared board it could just be a checkbox for "Due date notifications" without the need for a submenu.
Then the switch to a submenu with a "different" default setting wouldn’t be so strange, cause it’s more clear there’s a difference.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@jancborchardt I'll leave the animation for a later point in time, other comments in scope of the deck app should be addressed

Peek 2020-11-06 13-05

@juliusknorr juliusknorr force-pushed the enh/due-notify-settings branch from e6e5dd9 to 6b4d4d9 Compare November 6, 2020 12:06
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the enh/due-notify-settings branch from b2c2935 to a590144 Compare November 6, 2020 14:26
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@stefan-niedermann
Copy link
Member

@juliushaertl don't want to nag, but did you think about REST-API documentation (if this feature will also be available in the API)? 😇

@juliusknorr
Copy link
Member Author

@stefan-niedermann I already thought about you ❤️

b18ebe8

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.

Ability to choose to be notified per board
6 participants