Closed (fixed)
Project:
Affiliates Connect
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
29 May 2018 at 11:42 UTC
Updated:
29 Jun 2018 at 09:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
chiranjeeb2410 commentedPosting the current work shortly.
Comment #3
chiranjeeb2410 commentedFollowing patch contains the Affiliate Connect Config Settings from which every created plugin would intake some common config. Also includes the config schema and associated routes as well for the config form.
However, need to add the data cron storage dropdown containing additional configurations at which I'm currently at a fix. Will resolve that and add it asap.
Comment #4
chiranjeeb2410 commentedAttached screenshot of the current status of config page setup. Works fine for now. Adding data storage dropdown asap.
Comment #5
borisson_We should just remove this instead of commenting it.
The curly braces in other places in that class are not in the right place. Those should be fixed as well.
Comment #6
thedrupalkid commentedOne suggestion there..
Please have a help and config links in place by editing info.yml file for config.
configure - If your module offers a configuration form, then you can specify the route to this form here. It will then show up as a link in the Extend page (/admin/modules) when the user expands the details.
Ref: https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...
Comment #7
chiranjeeb2410 commented@borisson and @thedrupalkid, thanks for the suggestions. Would make it right away.
Comment #8
chiranjeeb2410 commented@borrison_ and @thedrupalkid,
Uploading complete Configuration setup form including all settings as required. Also, addressed all the issues as
suggested in #5 and #6. Attaching screenshots as well. Please review.
Comment #9
chiranjeeb2410 commentedComment #10
dbjpanda commented1- Use $this->t()
2- Better you can replace A,B,C,D.. with actual readable keys.
Comment #11
dbjpanda commentedComment #12
ankitjain28may commentedUse $this->t() in place of t()
Remove this line as it will ask for user input even if user has disabled the data storage.
Only this is required as it will check in case the user has enabled the field (data storage)
Same goes here
Same goes here
Same, change it too.
This too.
It should be 'color'
Save the cron form user input too as it's only saving 'data_storage' field.
Comment #13
chiranjeeb2410 commented@borrison_, @dbjpanda, @ankitjain28may,
The updated patch contains the completed and corrected code for the Configuration Settings page with all the
related route and schema/config data along with it. I have fixed all the issues as mentioned in the latest previous
comments regarding the form.
Also, the skeleton and the config page works smooth as of now without any errors. Attaching screenshots for the same.
Please review.
Comment #14
ankitjain28may commentedThis is not required as of now.
This is also not required.
'scrape_timer' field is missing.
This is showing error in schema file as in between `'` is not allowed.
default should be loaded from the congif like
#default_value' => $config->get('affiliate_import'),Add default value here too.. Else it will not load the user selected field.
And it would be better to use something like-
So that user can read & understand the content value too. Umm, that's a suggestion, I think it should be better to ask the advice of mentors.
Same goes here, Add default value here too.
Save the content of 'scrape_timer' field too.
Comment #15
chiranjeeb2410 commented@ankitjain28may,
Made all the required changes as mentioned in #14.
Please review.
Comment #16
chiranjeeb2410 commentedComment #17
ankitjain28may commented@chiranjeeb2410 You have attached the interdiff file with the name of the patch, pls add the patch as well as interdiff correctly.
Comment #18
ankitjain28may commentedAdd default_value field here too.
Comment #19
chiranjeeb2410 commentedPatch updated with all the requirements specified in #10, #12, #14 and #18 and attached along with
interdiff.
Up for review.
Comment #20
ankitjain28may commented@chiranjeeb2410, Patch looks good to me, Leaving it for mentors to review.
Comment #21
Lal_could you please add few more comments... :-)
Comment #22
Lal_Comment #23
chiranjeeb2410 commented@lal,
As in what. You need to be a bit more specific.
Comment #24
chiranjeeb2410 commentedUpdated patch as per suggestion in #21.
Please review.
Comment #25
dbjpanda commentedLooks good to me as well. Leaving for @borisson_ for a final review and commit.
Comment #26
dbjpanda commentedI have not tested it on local though. @ankit can you apply the patch on your local and check if it is working as expected or not .
Comment #27
ankitjain28may commentedThe patch is applied successfully, storing the user content correctly and working as expected, attached images for the same.
Comment #29
dbjpanda commentedThanks. I have committed the patch.
Comment #30
chiranjeeb2410 commented