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

amp-consent iframe modal UI #29204

Merged
merged 9 commits into from Aug 12, 2020
Merged

Conversation

micajuine-ho
Copy link
Contributor

@micajuine-ho micajuine-ho commented Jul 8, 2020

Overview:

  • Created new option for consent prompt iframe to appear as a modal rather than a bottom sheet, via promptUiSrc
  • modal mode will always include the overlay and force borders/box shadow
  • In the ready postMessage sent by the iframe, any vh value between (60-80] will trigger the new modal UI
  • Fullscreen requests from the iframe will be ignored if modal is enabled
  • Will send the Viewer a request to enter fullOverlay mode
  • Using CSS to create the modal effect (not the viewport service)
  • Initial Design Doc

Changes:

  • Change vh restriction received from the ready message from [30-60] to [30-80]
    • Values of (60-80] will trigger modalEnabled_ and overlayEnabled_
  • Create new CSS class: .modal and modal.enableBorder
    • .modal has !important styles to override some iframeActive rules
    • Use CSS var substitution (--i-amp-modal-height) set on the element style to be '${initialHeight}vh' to customize the height of the modal
    • Created borderEnabled to round all corners of the modal
  • Request fullOverlay from Viewer to hide Viewer header (similar to fullscreen mode)
  • If modalEnabled_, don't allow iframe to enter fullscreen
  • Onhide(), remove .modal & .borderEnabledModal
  • Tests:
    • show and hide for modal
    • no fullscreen
    • force overlay + borders
    • viewer messages being sent
  • Added documentation for initialHeight & enableBorder

Notes:

  • Default width: 90vw is used for the modal. Essentially, a margin of 5 on left and right to clearly show that the consent prompt is appearing above user content.
  • Max 80vh to show that consent prompt is appearing above user content (giving more room for underlying content to be shown). This was found to be a safe value when tested in all different browsers (no conflicts with navigation bar etc...). Additionally, there is no real difference between 80vh and 90vh unless all of the content will be shown in 90vh vs 80vh and thus no scrolling. Otherwise, user will still have to scroll down to the bottom to reach the consent decision buttons.

Normal Ampdoc:
ezgif com-video-to-gif (9)

Simulated Viewer:
ezgif com-video-to-gif (10)

Widescreen Desktop:
image

@micajuine-ho micajuine-ho changed the title [WIP] amp-consent iframe lightbox UI amp-consent iframe lightbox UI Jul 10, 2020
@micajuine-ho micajuine-ho requested a review from zhouyx Jul 10, 2020
extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
background: white;
flex-direction: column;
height: var(--lightbox-height);
width: 90vw;
Copy link
Contributor

@zhouyx zhouyx Jul 11, 2020

Choose a reason for hiding this comment

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

Out of curiously, how is the 90vw picked?

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 15, 2020

Choose a reason for hiding this comment

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

Confirmed with Zewen: We wanted a margin on the left and right to clearly show that the consent prompt is appearing above user content, which would be better UX.

Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

Have we tested desktop (wide viewports)? This might need a max-width in pixels.

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 29, 2020

Choose a reason for hiding this comment

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

Yes, Zewen said it is best practice to include a max-width so that the text isn't too stretched and hard to read. He suggested a value of 760px. Updated the description.

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
@hrkhal
Copy link
Contributor

hrkhal commented Jul 13, 2020

@micajuine-ho Why is the maximum height 80vh? Can this be increased to 90/95vh while additionally hiding the AMP viewer when the lightbox is launched?

Currently when we go into fullscreen we'd get 100vh and the AMP viewer would be hidden for maximum space.
#23491

With this PR as it stands we'd be gaining an extra 20vh for the initial prompt but then losing 20vh from the settings view of our CMP.

This is not an ideal compromise.

@micajuine-ho
Copy link
Contributor Author

micajuine-ho commented Jul 15, 2020

Hi @hrkhal Thanks for your feedback.

Why is the maximum height 80vh?

This is safe height for the consent iframe; it creates a large enough margin on the top and bottom (on all major browsers), to clearly show users that the consent prompt is above their content and that they must interact with it.

Can this be increased to 90/95vh

While we could increase the VH limit, the more we do so, the less apparent it will become to the user that their content is being blocked by this consent prompt, which would lead to a bad user experience. This problem is similar to issue #26197 (since lightbox mode would trigger without user interaction), and we would like to avoid that type of situation. Additionally, a change from 80vh to say 90vh should not have much impact, unless all the the disclaimers, information, and buttons will be shown in 90vh rather than 80vh.

while additionally hiding the AMP viewer when the lightbox is launched?

Thanks for the suggestion! I have edited the PR to include this.

With this PR as it stands we'd be gaining an extra 20vh for the initial prompt but then losing 20vh from the settings view of our CMP.

The intent of this feature is to allow a way for more information to be shown immediately on iframe loading, while maintaining a good UX. Can you elaborate on what you mean by "settings view".

Copy link
Contributor

@zhouyx zhouyx left a comment

Looks good to me!

A few things that I'd like the UI team to review

  1. Since this is a lightbox experience. Is it possible to reuse methods/css from amp lightbox?
  2. When the consent UI takes the entire screen, does it affect other UI component. For example I found amp-side-bar has a higher z index compared to amp-consent.

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
@hrkhal
Copy link
Contributor

hrkhal commented Jul 24, 2020

@micajuine-ho The settings view would be the second layer of a GDPR TCFv2 prompt where you're able to review your consent status for all of the vendors and purposes. See the below image.

Loosing height here makes an already cramped UX worse. We can obviously rejig the UI to make better use of the space but in general more height is preferable.

What's the eta for getting this into production? Cheers.

Screenshot 2020-07-24 at 17 25 24

@micajuine-ho
Copy link
Contributor Author

micajuine-ho commented Jul 24, 2020

The settings view would be the second layer of a GDPR TCFv2 prompt where you're able to review your consent status for all of the vendors and purposes. See the below image.

How would you get to this view? By hitting the manage button after making an initial consent decision?

What's the eta for getting this into production?

We are still waiting for review from the UI team (ping @alanorozco).

@hrkhal
Copy link
Contributor

hrkhal commented Jul 27, 2020

@micajuine-ho:

How would you get to this view? By hitting the manage button after making an initial consent decision?
Users can reach this via the the initial prompt as well as the manage button after an initial consent decision.

In our case the user would hit a cookie settings button (instead of a consent to all button) on the initial prompt that would allow them to configure their individual consent statuses before providing us with their overall consent.

We are still waiting for review from the UI team

Understood. I'd like to state that the deadline for TCFv2 is fast approaching (15th August 00:00 BST) and as a publisher we'll need a few days to implement this light box change and have it reviewed internally before we could get it to production. Ideally we'll be using this UI on day one.

Copy link
Member

@alanorozco alanorozco left a comment

Let's reconsider how this is done conditionally.

Maybe we can remove a couple of clauses for checking whether it's a lightbox, and use compound CSS selector for the styling props that change instead.

Consider also sharing class names that aren't necessary for either mode, but have no effect to reduce complexity.

@@ -118,17 +118,33 @@ amp-consent.i-amphtml-consent-ui-iframe-active {
transform: translate3d(0px, 100vh, 0px) !important;
}

amp-consent.i-amphtml-consent-ui-iframe-active.i-amphtml-consent-ui-enable-border {
amp-consent.i-amphtml-consent-ui-enable-border {
Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

I think for customization we should leave these classnames open (without i-amphtml) and use !important properties for whatever we find necessary. IMO, border-radius and box-shadow should be able to be modified.

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 29, 2020

Choose a reason for hiding this comment

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

Customization could be a nice feature for developers. I am down to address these changes in a future PR, since removing !important could be breaking.

background: white;
flex-direction: column;
height: var(--lightbox-height);
width: 90vw;
Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

Have we tested desktop (wide viewports)? This might need a max-width in pixels.

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
@@ -362,14 +372,20 @@ export class ConsentUI {
data['initialHeight'].indexOf('vh') >= 0
) {
const dataHeight = parseInt(data['initialHeight'], 10);
// Set initialHeight to max height fallback if applicable
this.initialHeight_ =
dataHeight >= 80 ? MAX_INITIAL_HEIGHT : this.initialHeight_;
Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

Why 80? Name this number as a constant if arbitrary, for future context.

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 28, 2020

Choose a reason for hiding this comment

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

80 is the MAX_INITIAL_HEIGHT agreed upon

Copy link
Member

@alanorozco alanorozco Jul 28, 2020

Choose a reason for hiding this comment

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

Meaning, maxInitialHeightForModal or how would you express it? It's just about giving it a name in the code.

extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
if (this.enableBorder_ && !this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorder);
} else if (this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorderLightbox);
this.sendViewerMessage_(REQUEST_OVERLAY);
Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

Flip to simplify conditional clause.

Suggested change
if (this.enableBorder_ && !this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorder);
} else if (this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorderLightbox);
this.sendViewerMessage_(REQUEST_OVERLAY);
if (this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorderLightbox);
this.sendViewerMessage_(REQUEST_OVERLAY);
} else if (this.enableBorder_) {
classList.add(consentUiClasses.enableBorder);

Copy link
Member

@alanorozco alanorozco Jul 27, 2020

Choose a reason for hiding this comment

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

  1. Also, consider flipping enableBorder_ to match lightboxEnabled_, that way you can use a compound selector and only send the message for lightbox.

  2. In any case, you should use the Viewport service for this, don't talk to the viewer directly.

Suggested change
if (this.enableBorder_ && !this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorder);
} else if (this.lightboxEnabled_) {
classList.add(consentUiClasses.enableBorderLightbox);
this.sendViewerMessage_(REQUEST_OVERLAY);
if (this.enableBorder_) {
// This classname is applied differently by CSS selector
classList.add(consentUiClasses.enableBorder);
}
if (this.lightboxEnabled_) {
this.viewport_.enterLightboxMode();
}

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 28, 2020

Choose a reason for hiding this comment

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

In any case, you should use the Viewport service for this, don't talk to the viewer directly.

Following the precedence set here #23491; basically this.viewport_.enterLightboxMode() does more work than we need.

Copy link
Member

@alanorozco alanorozco Jul 28, 2020

Choose a reason for hiding this comment

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

Which work is unnecessary? If it doesn't affect the use case negatively, I'd say it'd be better to consolidate implementations rather than duplicate them.

Copy link
Contributor Author

@micajuine-ho micajuine-ho Jul 28, 2020

Choose a reason for hiding this comment

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

Yeah, I agree.

@micajuine-ho micajuine-ho requested a review from alanorozco Jul 29, 2020
zhouyx
zhouyx approved these changes Jul 31, 2020
Copy link
Contributor

@zhouyx zhouyx left a comment

LG on my side. Please get approval on the UI changes from the UI team.

@micajuine-ho micajuine-ho changed the title amp-consent iframe lightbox UI amp-consent iframe modal UI Jul 31, 2020
Copy link
Member

@alanorozco alanorozco left a comment

Just nits, I think there are opportunities to simplify but overall LGTM.

extensions/amp-consent/0.1/amp-consent.css Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
extensions/amp-consent/0.1/consent-ui.js Outdated Show resolved Hide resolved
@micajuine-ho micajuine-ho merged commit 3970e5e into ampproject:master Aug 12, 2020
16 checks passed
ed-bird pushed a commit to ed-bird/amphtml that referenced this issue Dec 10, 2020
* init, no tests

* tests

* documentation + cleanup

* Suggested Changes

* Suggested Changes

* Lightbox => Modal, housekeeping, clarity

* Compound classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants