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

Added menu button to show a notification of the current pass for quick access #248

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

Conversation

daTom24
Copy link

@daTom24 daTom24 commented Mar 3, 2020

Added menu button to show a notification of the current pass for quick access
video: https://imgur.com/Ie456rB

@daTom24 daTom24 changed the title notification_branch Added menu button to show a notification of the current pass for quick access Mar 3, 2020
@simontb
Copy link
Contributor

simontb commented Mar 4, 2020

This looks cool! Thanks for the PR. I did not have a look at the code though. Does an issue exist where this feature is proposed/discussed? If yes, please link it in this PR.

I never saw something like this. Do you have any references for this kind of UX? I'd say it's quite unusual to trigger a notification manually. But I also don't know, if such a feature is needed overall and if yes whether the notification should be shown automatically.

Regarding automatic notifications: IIRC Apple Wallet shows a notification when you're close to a venue and have a valid pass. But this is just something that came to my mind right now. Not necessarily related to this PR.

@ligi
Copy link
Owner

ligi commented Mar 5, 2020

Thanks so much for the PR. But I agree with @simontb that I think this pattern is not very common. So I am a bit hesitant with merging. What I could imagine: A setting in each pass with "notification before" and then "always" could be one setting that basically is this feature you want. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants