iank Posted August 3, 2018 Share Posted August 3, 2018 Thanks @adrian - a bit late to the party today, but tested and all works fine, including iOS. 1 Link to comment Share on other sites More sharing options...
adrian Posted August 6, 2018 Author Share Posted August 6, 2018 On 7/25/2018 at 1:07 PM, tpr said: If the module is so tightly tied to the drupal module then converting the js from jquery to pure js would mean constantly tracking changes and too much possibility to fail somewhere in the conversion. The best would be then to ask the drupal devs if they are willing to drop jquery - if yes, we could help in the first step but after that they should be able to add updates (in pure js). I don't think every drupal site is using jquery, or is it so? Just to follow up on this - if you still feel like converting it to pure JS, I'd been keen to maintain that new version. I don't have the need or motivation to do it right now, but I can imagine that I may want it in the not too distant future. It doesn't sound like the Drupal version will end up being converted, but at this point in the development of the module, I think that's ok. If it receives any new killer features I'd be happy to port them over myself. Let me know if you're still on board. Thanks! 2 Link to comment Share on other sites More sharing options...
tpr Posted August 7, 2018 Share Posted August 7, 2018 Sure, I'll allocate some time to it, thanks for keeping this up. 3 Link to comment Share on other sites More sharing options...
tpr Posted August 8, 2018 Share Posted August 8, 2018 I've converted the js here (the original was renamed to CookieManagementBanner.jquery.js): https://github.com/rolandtoth/CookieManagementBanner I found some js parts was not used at all, eg. cookieMonster.cfg.block seems redundant (renamed to blockClass in my version), and the whole updateUi method also because there is no "theme" that uses the ".pwcmb--top_push" class (but correct me if I'm wrong). I removed the latter entirely. I have also made some minor CSS updates, eg. pointer cursor on labels and buttons. Hopefully I haven't break anything but a proper checking is needed (I don't use the module anywhere atm to test). 8 1 Link to comment Share on other sites More sharing options...
adrian Posted August 9, 2018 Author Share Posted August 9, 2018 Thanks @tpr - very generous of you to spend your time doing this! The ".pwcmb--top_push" class is no longer used - it was an option but it never seemed to work, so I removed the config option but never got around to cleaning up the other parts - thanks. Agreed, I don't see that cookieMonster.cfg.block being used anywhere. I have tasked someone else with testing this version - making sure all the GTM events fire as expected, along with the basic functionality, but if anyone here also wants to check, that would be awesome. Once we're sure everything is working, I'll incorporate into my repo (I'll get you to do a PR). Thanks again for this - I am sure many folks will be very appreciative of this new version! 2 Link to comment Share on other sites More sharing options...
tpr Posted August 9, 2018 Share Posted August 9, 2018 Thanks! I will probably do minor cosmetic changes to the code if I can get ESLint work with VSCode, but feel free to modify it if you find something. 2 Link to comment Share on other sites More sharing options...
tpr Posted August 9, 2018 Share Posted August 9, 2018 @adrian I've jshinted and formatted the code, you can update the test module if you wish. 2 Link to comment Share on other sites More sharing options...
iank Posted August 13, 2018 Share Posted August 13, 2018 @adrian Me again, sorry - I've just noticed something else a bit odd; I've used this in a multi-language environment, and see that the various fields for content and button text etc., are multi-language enabled. I haven't added any alternate language values for the text, but the banner doesn't seem to be picking up the default language values - instead I just see empty content areas and buttons for languages other than the default. See attached for English (default) and French versions of the same page. Default: French: I've only just noticed this, so it may be peculiar to this particular environment; I'll try it on a vanilla PW install and report back. Thanks, Ian. Link to comment Share on other sites More sharing options...
adrian Posted August 14, 2018 Author Share Posted August 14, 2018 Hi @iank - sorry about that - I honestly never tested this in a ML environment. Could you please check if the French values show if they are entered? If so, then we just need to make the fallback to default work if no alternate language values are entered. 1 Link to comment Share on other sites More sharing options...
iank Posted August 14, 2018 Share Posted August 14, 2018 Hi @adrian - no need to apologise! I can report a positive result though - the alternate language values are displayed if entered (see below). It appears to be just the fallback to default that's not being picked up. 1 Link to comment Share on other sites More sharing options...
adrian Posted August 14, 2018 Author Share Posted August 14, 2018 11 hours ago, iank said: Hi @adrian - no need to apologise! I can report a positive result though - the alternate language values are displayed if entered (see below). It appears to be just the fallback to default that's not being picked up. Thanks for testing. I have done the same here and can confirm your results. The problem is that I don't think PW has an inbuilt method for handling fallbacks for module settings. Because modules settings don't return an object you need to explicitly get the required value based on the user's language. I could write something custom in this module but I am curious if any of the ML experts out there have dealt with this when they have module config fields that have: 'useLanguages' => true Link to comment Share on other sites More sharing options...
tpr Posted August 14, 2018 Share Posted August 14, 2018 I remember having to manually resolve such issue somewhere but that doesn't mean there's no better way. Another question is whether to use automatic fallback or not, in case of native fields it's optional. If you bake it in the module the user cannot set a language value empty intentionally because on retrieval it will be replaced by the default one. 1 Link to comment Share on other sites More sharing options...
adrian Posted August 14, 2018 Author Share Posted August 14, 2018 Just now, tpr said: I remember having to manually resolve such issue somewhere but that doesn't mean there's no better way. Another question is whether to use automatic fallback or not, in case of native fields it's optional. If you bake it in the module the user cannot set a language value empty intentionally because on retrieval it will be replaced by the default one. I had the same thought actually - thanks for bringing it up here. I do feel like in particular with module settings that it's something that shouldn't be hard to populate all languages with - it's not the same as new pages added to a site where you really can't enforce a site editor to populate all the alternate language values. Link to comment Share on other sites More sharing options...
iank Posted August 15, 2018 Share Posted August 15, 2018 16 hours ago, adrian said: it's something that shouldn't be hard to populate all languages with - it's not the same as new pages added to a site where you really can't enforce a site editor to populate all the alternate language values. @adrian That's true, I guess. In my case though, the site I've just launched and am using this on has news articles that may be populated with alternate language field values, though in the main, they are just in the default (English). Where one or more alternative language variant exists for an article, we render links to those alternate language variations. Visiting such a link switches the user's language setting so they can view any translated content in that language. Not quite the way Processwire's Language functionality was designed to be used I know, but it fits the client's needs. Since the site's now live I can show you the example alluded to in the screenshots in my earlier posts. If you visit https://www.traffic.org/news/kota-kinabalu-hosts-heart-of-borneo-judiciary-workshop/ and before accepting the cookie notice, click the "Francais" link, you'll see the problem. Such an article might have content in French, Spanish, Vietnamese, Japanese or Chinese (or any combination), so I'd need to get the module settings text translated for each of these, and remember to do this if we add new languages further down the line. However, a thought just occurred to me as I was writing this - the above site is heavily cached with ProCache. If a user from France say, visits a yet uncached page (say an 'ordinary' page without any language versions), I presume the page then be cached with blank (or translated) values for the cookie banner text etc.? Subsequent visits, regardless of the user's language, will get ProCache's static html version. Hmmm.... ? Link to comment Share on other sites More sharing options...
adrian Posted August 15, 2018 Author Share Posted August 15, 2018 Hi @iank - sorry, I have no idea how ProCache will handle this - I have never used it. Regarding dealing with fallback if empty - I think the only way to logically do this and also consider @tpr's point about allowing for an intentional blank value is maybe to only fallback when a field is required. I think that will work fine for this module. What do you both think? Link to comment Share on other sites More sharing options...
iank Posted August 15, 2018 Share Posted August 15, 2018 Hi @adrian: Yes, I think that would work. At least then there is always something visible to the user for the key fields and button texts. I wonder if the cache thing may be an issue though, even if using template cache or WireCache. I'm not sure if there's an easy way around this. Sorry to keep adding problems Adrian.. Link to comment Share on other sites More sharing options...
tpr Posted August 15, 2018 Share Posted August 15, 2018 For me it's ok. As an alternative possible workaround could be entering "=" to the lang field where you enable fallback, kinda like in the Language Translator. Or entering "empty" where no fallback is allowed but that is less pw-ish. Link to comment Share on other sites More sharing options...
adrian Posted August 15, 2018 Author Share Posted August 15, 2018 Honestly I am starting to feel like this is something that should be solved by the PW core - I don't really feel like messing around with hacky solutions for this. @iank - would you be willing to submit an issue to the PW github account? I am honestly not sure if this should be an issue or a request, but I feel like it's strange that normal fields allow for an easy fallback, but module settings don't, so maybe it's an issue? Link to comment Share on other sites More sharing options...
iank Posted August 16, 2018 Share Posted August 16, 2018 Hi @adrian - I've submitted an issue to the PW issues github. As you say, it doesn't quite feel like a bug, but at least it's officially on record now. 2 Link to comment Share on other sites More sharing options...
gebeer Posted August 19, 2018 Share Posted August 19, 2018 Hello, I just installed this module on PW 2.7.2. On the module settings page I get an error "Call to a member function render() on a non-object" from line 78 of CookieManagementBanner.module. $this->wire('files') is null. If I replace line 78 with the following, everything works ok. $cookieBanner = wireRenderFile($this->wire('config')->paths->$this.'wrapper.tpl.php', array('module' => $this, 'lang' => $lang)); Seems like wire('files') is not available in 2.7.2 which is strange. Can anyone confirm this? Link to comment Share on other sites More sharing options...
gebeer Posted August 19, 2018 Share Posted August 19, 2018 I've been following the discussion about language translation for this module. It is convenient to edit the language specific texts on the module settings page. But if they were implemented as i18n language translation strings, I guess all those problems would be solved. I personally wouldn't mind to edit those under Settings->Languages as long as there is a hint on the module settings page. Link to comment Share on other sites More sharing options...
adrian Posted August 19, 2018 Author Share Posted August 19, 2018 6 hours ago, gebeer said: Hello, I just installed this module on PW 2.7.2. On the module settings page I get an error "Call to a member function render() on a non-object" from line 78 of CookieManagementBanner.module. $this->wire('files') is null. If I replace line 78 with the following, everything works ok. $cookieBanner = wireRenderFile($this->wire('config')->paths->$this.'wrapper.tpl.php', array('module' => $this, 'lang' => $lang)); Seems like wire('files') is not available in 2.7.2 which is strange. Can anyone confirm this? Thanks @gebeer - I forgot about that. I have pushed a new version that uses wireRenderFile instead. It definitely wasn't available in older versions of PW. 1 Link to comment Share on other sites More sharing options...
adrian Posted August 19, 2018 Author Share Posted August 19, 2018 5 hours ago, gebeer said: But if they were implemented as i18n language translation strings, I guess all those problems would be solved. Would the problems be solved though? I don't use the multi-language stuff enough to know exactly how these settings would be handled when used on the frontend like this. If someone feels like experimenting with this, that would be great! Link to comment Share on other sites More sharing options...
adrian Posted August 21, 2018 Author Share Posted August 21, 2018 Just wondering if anyone out there has tested @tpr's vanilla JS version: https://github.com/rolandtoth/CookieManagementBanner - it would be great to get it merged fairly shortly as I don't think any PW frontend modules should require jquery. Thanks for testing and letting my know if you find any problems. Link to comment Share on other sites More sharing options...
ceberlin Posted August 23, 2018 Share Posted August 23, 2018 on localhost, I get a warning: Notice: Undefined index: countryCode in /Users/.../modules/CookieManagementBanner/CookieManagementBanner.module on line 42 Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now