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

feat(useWebNotification): add requestPermissions option, return permissionGranted and ensurePermissions #3325

Merged
merged 3 commits into from Aug 25, 2023

Conversation

michealroberts
Copy link
Contributor

@michealroberts michealroberts commented Aug 18, 2023


Description

This MR addresses the usage concerns in #3314

This MR does not introduce breaking changes to the usage of the functional API, but introduces a change in the triggering of the notification request permission, i.e., it will no longer request permission onMounted.


🤖 Generated by Copilot at 093dd7a

This pull request enhances the useWebNotification hook with a new option and helper functions to request permission for web notifications more flexibly and conveniently. It also updates the documentation with an example of using the new option.

🤖 Generated by Copilot at 093dd7a

  • Add requestOnMounted option to useWebNotification hook to control when permission is requested (link,link,link,link)
  • Simplify permission checking and requesting logic using hasPermission and requestPermission functions (link,link,link)
  • Refactor defaultOptions to defaultWebNotificationOptions and destructure window option (link,link)
  • Update useWebNotification/index.md with new option and functions (link)

…on request onMounted

refactor: amended useWebNotification() to optionally specify permission request onMounted
@michealroberts michealroberts changed the title refactor: amended useWebNotification() to optionally specify permission request onMounted refactor(useWebNotification): amended to optionally specify permission request onMounted Aug 18, 2023
@antfu
Copy link
Member

antfu commented Aug 25, 2023

Updated the interface to align with https://vueuse.org/core/useDevicesList/#usedeviceslist,

Also set the value default to true to avoid breaking

@antfu antfu changed the title refactor(useWebNotification): amended to optionally specify permission request onMounted feat(useWebNotification): add requestPermissions option, return permissionGranted and ensurePermissions Aug 25, 2023
@antfu antfu enabled auto-merge August 25, 2023 11:41
@antfu antfu added this pull request to the merge queue Aug 25, 2023
Merged via the queue into vueuse:main with commit a1753d9 Aug 25, 2023
4 checks passed
@SampsonCrowley
Copy link
Contributor

@michealroberts I'm a little late to the party but I think there's a typo here

if (!isSupported.value)
// If either the browser does not support notifications or the user has
// not granted permission, do nothing:
if (!isSupported.value && !permissionGranted.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be ||

if (!isSupported.value || !permissionGranted.value)

@SampsonCrowley
Copy link
Contributor

#3422

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.

None yet

3 participants