Skip to content

Make privacy consent dialog scrollable on mobile #3507

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

Merged

Conversation

daphneslootmans
Copy link
Contributor

Type

  • Enhancement

Pull request description

Make the privacy consent dialog scrollable on mobile

@daphneslootmans daphneslootmans requested a review from a team as a code owner April 27, 2022 09:28
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Base: 27.87% // Head: 27.86% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (8b7e934) compared to base (a2c3644).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3507      +/-   ##
============================================
- Coverage     27.87%   27.86%   -0.02%     
  Complexity     8150     8150              
============================================
  Files           575      575              
  Lines         30706    30706              
============================================
- Hits           8560     8556       -4     
- Misses        22146    22150       +4     
Flag Coverage Δ
functional 23.77% <ø> (-0.02%) ⬇️
installer 3.84% <ø> (ø)
unit 7.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Frontend/Core/Language/Language.php 51.66% <0.00%> (-3.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

overflow: initial;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

you need to add a newline at the end of your file here

@@ -1,6 +1,6 @@
{% include "Core/Layout/Templates/Head.html.twig" %}

<body class="{{ LANGUAGE }}" itemscope itemtype="http://schema.org/WebPage">
<body class="{{ LANGUAGE }} {% if not privacyConsentDialogHide %}modal-open{% endif %}" itemscope itemtype="http://schema.org/WebPage">
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why you have the class modal-open here? if it has to do with javascript you should use a data attribute. On top of that I'm not really a fan of having to add something there in the templates since that should be handled by the js itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To block the body from scrolling when the modal is open and scrollable. This way the class is only added when the consent dialog is visible. The consent dialog html is always rendered and just set to display: none when it doesn't need to be shown, so the element is always there.
I can change it so the modal html is only added when privacyConsentDialogHide = false and add the class to the body with js instead if you prefer.

@@ -1,6 +1,6 @@
{% include "Core/Layout/Templates/Head.html.twig" %}

<body class="{{ LANGUAGE }}" itemscope itemtype="http://schema.org/WebPage">
<body class="{{ LANGUAGE }} {% if not privacyConsentDialogHide %}modal-open{% endif %}" itemscope itemtype="http://schema.org/WebPage">
Copy link
Member

Choose a reason for hiding this comment

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

idem

@carakas carakas added this to the 5.11.2 milestone Apr 27, 2022
@carakas carakas self-assigned this Apr 27, 2022
@daphneslootmans
Copy link
Contributor Author

@carakas changes made as requested, can you check again please

@daphneslootmans
Copy link
Contributor Author

@carakas changes made as requested, can you check again please

@carakas
Copy link
Member

carakas commented Oct 1, 2022

@daphneslootmans sorry for the delay, I was out for almost the whole month of September because of Covid

@carakas carakas merged commit 7f67f74 into forkcms:master Oct 1, 2022
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.

2 participants