This repository was archived by the owner on May 10, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e72549d
to
733cac0
Compare
cf365a3
to
370e45e
Compare
baf4e85
to
430ba23
Compare
9774b32
to
62efd3a
Compare
430ba23
to
e1f3b09
Compare
62efd3a
to
c858cc1
Compare
e1f3b09
to
4ab9993
Compare
01a8bb2
to
f5f547a
Compare
4ab9993
to
b126c68
Compare
e1f6ef0
to
7cb4a0f
Compare
44b5300
to
a236c6c
Compare
24bcf0c
to
d6ddfbc
Compare
a236c6c
to
7ce87b3
Compare
e687f94
to
7d5bd74
Compare
Client/Frontend/Settings/BraveShieldsAndPrivacySettingsController.swift
Outdated
Show resolved
Hide resolved
...rontend/Browser/BrowserViewController/BrowserViewController+CookieNotificationBlocking.swift
Show resolved
Hide resolved
soner-yuksel
approved these changes
Sep 27, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments are fixed and working as expected 🎉
iccub
suggested changes
Sep 28, 2022
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Settings/BraveShieldsAndPrivacySettingsController.swift
Outdated
Show resolved
Hide resolved
11 tasks
I have another question. are we supposed to show this pop-up for new users ? |
49a147f
to
b25a4d1
Compare
iccub
suggested changes
Sep 30, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SwiftUI changes, and the new swiftUI does not support accessibility, it has to be fixed
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
4802098
to
6023618
Compare
iccub
reviewed
Oct 3, 2022
Client/Frontend/Popup/CookieNotificationBlockingConsentView.swift
Outdated
Show resolved
Hide resolved
Most of my comments are still unaddressed |
iccub
suggested changes
Oct 5, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, only thing left is the BraveButton style changes
295edc0
to
3fb68d9
Compare
iccub
approved these changes
Oct 28, 2022
3fb68d9
to
2b20c84
Compare
soner-yuksel
approved these changes
Nov 2, 2022
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of Changes
This will add a setting to Shields to enable cookie consent notices
It will also show a popup on second launch to allow the user to know this feature is available
This pull request fixes #1358
Submitter Checklist:
NSLocalizableString()
Test Plan:
Make sure that
When the user clicks on "Yes block cookie consent notices" on the popup:
When the user clicks on "No thanks" on the popup
When the setting is enabled:
When the setting is not enabled:
Persistence
Screenshots:
Reviewer Checklist:
QA/(Yes|No)
bug
/enhancement