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 and enable gtag #922

Merged
merged 19 commits into from
Oct 25, 2022
Merged

Add cookie banner and enable gtag #922

merged 19 commits into from
Oct 25, 2022

Conversation

e111077
Copy link
Contributor

@e111077 e111077 commented Aug 30, 2022

Fixes #932

We should first enable page count analtics

Links to test:

light bg:
https://pr922-8f2ca78---lit-dev-5ftespv5na-uc.a.run.app/

dark bg:
https://pr922-8f2ca78---lit-dev-5ftespv5na-uc.a.run.app/playground
https://pr922-8f2ca78---lit-dev-5ftespv5na-uc.a.run.app/tutorials/intro-to-lit

Followed semantics and DOM structure of the angular and firebase ones

@e111077
Copy link
Contributor Author

e111077 commented Aug 31, 2022

oops, screenshot tests. I mean what's so bad about this test result?

inheritingStylesPlaygroundPreview-linux

@aomarks
Copy link
Member

aomarks commented Aug 31, 2022

I think it looks a bit weird on the playground and tutorials pages, because the background color is the same as the one on that page. What if it was black?

@aomarks
Copy link
Member

aomarks commented Aug 31, 2022

I think it looks a bit weird on the playground and tutorials pages, because the background color is the same as the one on that page. What if it was black?

A shadow could also help. The one at https://web.dev/ is black with shadow.

@e111077 e111077 marked this pull request as ready for review August 31, 2022 00:45
@e111077
Copy link
Contributor Author

e111077 commented Aug 31, 2022

OMG TESTS PASS! I've also added a screenshot test for the banner. Also added Material elevation 5 following material.io's recommendation for material 2 snackbars

@aomarks
Copy link
Member

aomarks commented Aug 31, 2022

Curious where did the language for this come from? The web.dev one is "We serve cookies on this site to analyze traffic, remember your preferences, and optimize your experience."

@aomarks
Copy link
Member

aomarks commented Aug 31, 2022

What if it was black?

How did black look?

@e111077
Copy link
Contributor Author

e111077 commented Aug 31, 2022

oh oops here's the black one. Material design tends to add an overlay opacity on top of elevated items. This makes it lighter as it approaches... the sun? the screen? but its the same underlying bg color. Want me to try that?

image

@augustjk
Copy link
Contributor

Curious where did the language for this come from?

Yes. Do we need to say cookies from Google?

@e111077 e111077 requested a review from aomarks August 31, 2022 03:37
@e111077 e111077 self-assigned this Aug 31, 2022
@e111077 e111077 changed the title Add banner notifying of gtag use Add banner notifying of gtag use [DO NOT MERGE] Sep 1, 2022
@e111077 e111077 changed the title Add banner notifying of gtag use [DO NOT MERGE] Add cookie banner [DO NOT MERGE] Sep 1, 2022
@e111077 e111077 changed the title Add cookie banner [DO NOT MERGE] Add cookie banner and reenable gtag Oct 25, 2022
@e111077 e111077 requested a review from aomarks October 25, 2022 21:06
@e111077 e111077 changed the title Add cookie banner and reenable gtag Add cookie banner and enable gtag Oct 25, 2022
@e111077
Copy link
Contributor Author

e111077 commented Oct 25, 2022

We finally got the go-ahead from launch review

Co-authored-by: Alexander Marks <aomarks@google.com>
@e111077 e111077 requested a review from aomarks October 25, 2022 21:14
Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Screenshot needs updating I assume?

@e111077
Copy link
Contributor Author

e111077 commented Oct 25, 2022

IDK why the tests keep getting cancelled

@e111077
Copy link
Contributor Author

e111077 commented Oct 25, 2022

test failures are from another PR that got into main. Will fix in followup

@e111077 e111077 merged commit 74e4ca6 into main Oct 25, 2022
@e111077 e111077 deleted the gtag-banner branch October 25, 2022 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable page count analytics + cookie banner
3 participants