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

Add permission request to settings #3649

Merged
merged 2 commits into from Apr 16, 2023
Merged

Add permission request to settings #3649

merged 2 commits into from Apr 16, 2023

Conversation

yayuyokitano
Copy link
Member

Adds a popup-esque thing on top of settings that asks the user to give host permissions if they didn't already. Helps a bit with UX, since this permission is no longer automatically granted in firefox/safari with manifest V3.

image

@yayuyokitano yayuyokitano added new-feature For PRs that add new functionality minor-change For minor changes labels Apr 16, 2023
@yayuyokitano
Copy link
Member Author

As some context on why I have made the decision I have made regarding how to trigger, browser.permissions.request must be triggered by a user action

hasPermissions(setPerms);

return (
<Show when={!perms()}>
Copy link
Member

Choose a reason for hiding this comment

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

How does this work? how does perms() know what perms are set?

Copy link
Member Author

Choose a reason for hiding this comment

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

perms is the getter function of a signal

The signal is set to true by default, then the signal setter setPerms is uses to set it to false if perms are not granted, and set back to true if the user grants permission.

Signals allow the component to be reactive – solid only renders the component once, so if you just use a raw variable it will not change what is rendered when you change the variable value. Signals (and other similar solid utilities) tell the component to actually change what is rendered too.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation thought it was something like that :)

@inverse inverse merged commit 47e3068 into web-scrobbler:master Apr 16, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor-change For minor changes new-feature For PRs that add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants