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

Create Modal for Brave Browser notification #1180

Merged
merged 8 commits into from
Aug 10, 2022
Merged

Create Modal for Brave Browser notification #1180

merged 8 commits into from
Aug 10, 2022

Conversation

wirednkod
Copy link
Contributor

@wirednkod wirednkod commented Jul 29, 2022

This PR creates the notification inside Popup and Options (only in Brave Browser) about the socket restriction that was introduced in Brave Browser in version 1.36.109.

Final design details are still pending.
Fixes #1178

A new PR will open to adjust the design when it is provided.

Popup outcome:
Screenshot 2022-07-29 at 6 25 27 PM

Options outcome:
Screenshot 2022-07-29 at 6 25 09 PM

@wirednkod wirednkod marked this pull request as ready for review August 5, 2022 11:31
@wirednkod wirednkod requested a review from tomaka August 5, 2022 11:32
@wirednkod wirednkod marked this pull request as draft August 5, 2022 12:32
@wirednkod
Copy link
Contributor Author

Some extra info: Dismiss button is adding an entry in localStorage for the note to stop appearing.
Learn more should lead to Landing Page section that will explain what happened with Brave Browser

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

I'm not a fan of using localStorage directly from within UI components. This is spaghetti-ish. But I don't want to go back and forth with review comments.

@wirednkod
Copy link
Contributor Author

I'm not a fan of using localStorage directly from within UI components. This is spaghetti-ish. But I don't want to go back and forth with review comments.

I see what you mean - will transfer it to the background. Thank you

@wirednkod wirednkod added the automerge Instructs Mergify to queue this pull request for automatic merging label Aug 10, 2022
@mergify mergify bot merged commit 370f25d into main Aug 10, 2022
@mergify mergify bot deleted the nik-brave-modal branch August 10, 2022 18:18
@tomaka
Copy link
Contributor

tomaka commented Aug 11, 2022

Your last commit is actually IMO worse than what there was before.
The problem of "using localStorage directly" has nothing to do with using localStorage directly, it has to do with using localStorage.
The code of the UI component shouldn't contain any reference to the word "local storage". It should query the state of this modal popup, and it's background.ts that decides that this state is stored in the local storage.

@wirednkod
Copy link
Contributor Author

Your last commit is actually IMO worse than what there was before. The problem of "using localStorage directly" has nothing to do with using localStorage directly, it has to do with using localStorage. The code of the UI component shouldn't contain any reference to the word "local storage". It should query the state of this modal popup, and it's background.ts that decides that this state is stored in the local storage.

The wording in your comment, sounded like the UI components should not access chrome.storage at all, rather than "querying" the state of this modal popup, and it's 'background.ts' that decides that this state is stored in the local storage. Comment above makes clearer what you actually meant. Will alter in a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instructs Mergify to queue this pull request for automatic merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement UI for Restrict WebSockets pool on Brave Browser
2 participants