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 cookie banner #3097

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Add cookie banner #3097

merged 2 commits into from
Nov 26, 2024

Conversation

mit-mit
Copy link
Member

@mit-mit mit-mit commented Nov 25, 2024

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ✔️
File Coverage

This check for test coverage is informational (issues shown here will not fail the PR).

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/samples/lib/brick_breaker.dart
pkgs/samples/lib/fibonacci.dart
pkgs/samples/lib/google_ai.dart
pkgs/samples/lib/hello_world.dart
pkgs/samples/lib/main.dart
pkgs/samples/lib/sunflower.dart

@mit-mit
Copy link
Member Author

mit-mit commented Nov 25, 2024

Screenshot 2024-11-25 at 15 44 07

@johnpryan
Copy link
Contributor

johnpryan commented Nov 25, 2024

Thanks for sending a pull request - I tested this locally and deployed this to https://sketch-pad-1cebb--cookie-banner-bvpssvbh.web.app, but I'm not seeing the cookie banner.

cc/ @devoncarew I remember you mentioning there were some technical issues you noticed when you tried to add this feature?

@devoncarew
Copy link
Member

cc/ @devoncarew I remember you mentioning there were some technical issues you noticed when you tried to add this feature?

I was not able to see the banner when I tried this myself, a few months ago. If I remember correctly, I think that the flutter content was always full screen - the banner didn't have any height, or was obscured?

Perhaps I was missing something, or the banner has some other check that wasn't triggering. From the screenshot @mit-mit was able to get it working.

@johnpryan
Copy link
Contributor

I think this is because I am in the US.

The Cookie Notification Bar will only be shown if the user is visiting from an eligible region, so will not be visible if testing from elsewhere (eg. United States).

@mit-mit
Copy link
Member Author

mit-mit commented Nov 26, 2024

Correct, the banner is geo-coded to only display in eligible regions.

Googlers can use go/polyjuice-extension to test the behavior in other regions than their physical one.

@mit-mit
Copy link
Member Author

mit-mit commented Nov 26, 2024

@johnpryan can you try to test with that and confirm that its working on your end on your staged site? If so, please go ahead and merge.

@johnpryan
Copy link
Contributor

LGTM

@johnpryan johnpryan merged commit cad6544 into dart-lang:main Nov 26, 2024
9 checks passed
@mit-mit mit-mit deleted the cookiebanner branch November 27, 2024 08:29
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.

3 participants