The 'Accept All' and Deny All' buttons of the Cookie Settings modal are visible on Desktop and Tablet screens, but those are not showing on mobile devices, hence not giving the user any option to accept of deny the cookie from the modal itself if he/she is viewing it on small screen.
1. Install and Configure the module as per need.
2. Place the block on the footer region.
3. Click on the Cookie Settings button which will open up the modal.
4. All the buttons ('Accept All'. 'Deny All', 'Save') are visible on desktop and tablet screens.
5. Now switch to responsive screen of mobile devices.
6. The 'Accept All' and 'Deny All' button will disappear.
Desktop View:
Tablet View:
Mobile View:
Make the buttons visible on mobile devices by removing display: none property from the modal buttons and add suitable style for the same.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | after_spacing_issue.png | 111.76 KB | esha_kundu |
| #20 | Screenshot from 2024-10-04 14-06-11.png | 124.99 KB | sourojeetpaul |
| #20 | 3457021-20.patch | 23.9 KB | sourojeetpaul |
| #19 | spacing_issue.png | 110.12 KB | esha_kundu |
| #19 | alignment_fixes.png | 128.28 KB | esha_kundu |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
sourojeetpaul commentedWorking on it!
Comment #3
anybodyThanks @sourojeetpaul!
@thomas.frobieter could you perhaps take a look at this in the next weeks? (For review or implementation)
Comment #4
anybodyComment #5
sourojeetpaul commentedHello @Anybody,
Thanks for the prompt response, but I'm not sure why I've been unassigned from the issue.
I've already started working on it, just got occupied on something else in between.
Anyways, as per my latest findings, the issue seems to be coming from the cookiesjsr library that the module is dependent upon. Hence I think it'll be better if we address the issue as a part of the library only, so that if the library gets used on somewhere else apart from this module, it'll also mitigate the same issue over there as well.
So I created an issue on the github repo of the library
https://github.com/jfeltkamp/cookiesjsr/issues/25
I've also prepared the solution which address the issue correctly, but unfortunately I'm not able to raise PR against it, as it seems that the repo might be maintaining some sort of strict push access policy. It'll be great if you can grant me push access to that repo, so that I can raise the PR and you guys can have a look at it!
Comment #6
sourojeetpaul commentedHi,
As I don't have the push access on the libraries repo, hence I'm giving a patch file over here. On applying it over the cookiesjsr library the issue is getting resolved. I'll create the corresponding PR for the same on that github repo once I get required permissions to do so.
Also I'm attaching before and after screenshots for reference!
Comment #7
sourojeetpaul commentedComment #8
anybody@sourojeetpaul simply click "Get push access" above and create a MR please.
Comment #9
sourojeetpaul commentedHi @Anybody,
I'm not talking about getting push access on this issue's forked repo on gitlab. I already have a push access for the same.
As I've already mentioned on #5 I've raised an issue on the Cookiesjsr library Github repo itself
https://github.com/jfeltkamp/cookiesjsr/issues/25
I'm getting the push access issue on the Github repo, and as I can see @JFeltkamp is the contributor of the repo, so I'd request if he/anyone can grant me push access on that repo.
Thanks!
Comment #10
anybody@sourojeetpaul aaah sorry I got you wrong! Yeah that's something @JoachimFeltkamp has to have a look at. Maybe you can send him a PM?
Comment #11
sourojeetpaul commentedOk sure, I'm trying to get in touch with him via Drupal Slack!
Comment #12
jfeltkampHey @sourojeetpaul, I'm here :-)
The missing buttons in mobile layouts are technically and legally not required, but general shortcuts for "configure all/none + save".
The reason why the buttons are missing is the missing space for button labels.
In English everything looks fine, but if you translate the plugin to German, then:
Save | Deny all | Accept all
Speichern | Alle ablehnen | Alle akzeptieren
This will break the layout.
If you have a good solution for that problem I would be pleased.
Comment #13
sourojeetpaul commentedHi @JFeltkamp,
Thanks for the prompt reply, and indeed its breaking when we change the language to german or something else where the number of letters are increasing. Sorry for the overlook!
One solution would be to change the flex layout at smaller screens like that of the primary cookies panel.
Thanks for pointing that out, I'm working on it and providing the solution ASAP!
Comment #14
sourojeetpaul commentedHello there,
According to the feedback and as per my findings as well, I'm providing a new patch over here, which will hopefully resolves the issue.
As mentioned on my previous comment, on changing the flex layout to that of the primary cookies panel, things will look good.
Removing options from the user who're viewing it from smaller devices, and force them to select or deselect cookies by going back to the primary panel would be little bit injustice I think :)
Pls have a look at this new patch, and do let me know if it looks good to you.
Attaching screenshots for reference!
Comment #15
riddhi.addweb commentedI tried to apply the patch 3457021-14. but seems like it does not resolve the issue. The patch issue persists and also on the Desktop and Mobile view the Texts are overlapping on the card. Attaching the SS for the same.
Comment #16
sourojeetpaul commentedHi @riddhi.addweb,
The text overlapping issue that you've mentioned in your previous comment is legit and I've also experienced the same, but that's a completely separate issue to discuss over here. The text overlapping issue is happening with Drupal's default Olivero theme, but doesn't happen with Bootstrap. I'm yet to test the same on other themes to come to a conclusion. So after my investigation I'll raise a new issue for that and try to address that on that ticket only.
As far as this issue is concerned, try to follow the provided "Steps to Reproduce" on Bootstrap5 theme, I hope you'll be able to get what I'm talking about in this issue.
And for the patch, as I've mentioned explicitly on comment #6 the issue is of the library and not of the module, so to resolve the issue you've to apply the patch on the cookiesjsr library which comes with the module, and not directly on the module. I think you've tried to apply the patch conventionally on the module itself, so it won't work here like that. Pls go through the previous threads to get a better understanding of the same.
Thanks :)
Comment #17
esha_kundu commentedHello,
I've reviewed the issue by following the below steps:
I would like to point out two design fixes that needs to be rectified,
Hence, moving to NW.
Comment #18
sourojeetpaul commentedHi @esha_kundu,
Thanks for the extensive review and pointing out the minute style breaks. I've addressed these on my latest patch which I'm providing here now, please have a look once again! I've tested the same on Japanese as well as you've suggested, hope it'll solve the said issue :)
Attaching screenshots for better reference!
Comment #19
esha_kundu commentedHello @sourojeetpaul,
I've re-reviewed by applying the patch 3457021-18.patch. The design fixes that I had mentioned in #17 was addressed, which resolved the incorrect alignment and the smaller width of save button in landscape view of mobile devices. However, it seems like there is still one design issue that is not addressed. In devices having width greater than 576px the buttons are having equal space between them, but the save button is not equally spaced below 576px. I've provided ss for reference.
Please have a look.
Comment #20
sourojeetpaul commentedHi @esha_kundu,
Thanks for re-reviewing my patch so minutely, I've accidentally missed that. Can you pls have a look once again!
Attaching screenshot for quick ref.
Comment #21
esha_kundu commentedHello @sourojeetpaul,
The issue that I had mentioned previously in #19 has been resolved with your current patch 3457021-20.patch hence moving to RTBC. Attaching ss for reference.
Thanks