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

Support Actions #15

Merged
merged 21 commits into from Jun 2, 2021
Merged

Support Actions #15

merged 21 commits into from Jun 2, 2021

Conversation

danirabbit
Copy link
Member

@danirabbit danirabbit commented Jun 12, 2019

Fixes #1

@danirabbit danirabbit marked this pull request as ready for review June 12, 2019 22:34
@donadigo
Copy link
Contributor

Something isn't right here:
notification

The labels are flipped with the action ID's and the layout is kind of screwed.

Copy link
Contributor

@donadigo donadigo left a comment

Choose a reason for hiding this comment

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

Has issues.

@danirabbit danirabbit requested a review from donadigo June 15, 2019 14:34
Copy link
Contributor

@donadigo donadigo left a comment

Choose a reason for hiding this comment

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

  1. If the caller doesn't provide a desktop-entry but provides valid actions, those don't display but should, because they don't need any app info to work.

  2. Should the icon be valigned to top like this? Wouldn't it make more sense to have it aligned in the center?
    notification

@danirabbit
Copy link
Member Author

Screenshot from 2019-06-16 20 23 24@2x

I guess we have to figure out what to do about this ^

@danirabbit danirabbit mentioned this pull request Jun 23, 2019
@danirabbit danirabbit added this to In progress in 6.0 Odin Release via automation Jan 14, 2020
@danirabbit danirabbit removed this from In progress in 6.0 Odin Release Jan 14, 2020
@danirabbit danirabbit added this to In progress in Native Notifications via automation Jan 14, 2020
@bluesabre
Copy link
Sponsor Member

@danrabbit I hacked on this a bit today, and I've got the actions working on top of the latest master.
https://github.com/bluesabre/notifications/tree/actions

I've made the following additional changes:

  • Made the close button revealer only use the space in the corner to make the default and action buttons clickable again.
  • Switched to just using the notification actions, which should make them behave as expected for the sending applications.
  • Enabled action support for notifications without app_info.

Let me know if you'd like me to make any additional changes or to continue working on this.

@danirabbit
Copy link
Member Author

@bluesabre Yes if you can propose that PR that would be great to move forward on this :)

@bluesabre
Copy link
Sponsor Member

@bluesabre Yes if you can propose that PR that would be great to move forward on this :)

Awesome. Opened up a PR for the actions branch here: #125

@bluesabre
Copy link
Sponsor Member

@danrabbit Everything looks good to me, thanks!

@danirabbit danirabbit merged commit eb6e658 into master Jun 2, 2021
@danirabbit danirabbit deleted the actions branch June 2, 2021 01:26
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.

Implement default_action
4 participants