While evaluating this module, I noticed the readme's configuration paths are incorrect and that there are some minor typos. I also think the file should be changed to a .md file to utilize markdown. Patch to follow, thanks!!

Comments

volkswagenchick created an issue. See original summary.

volkswagenchick’s picture

Assigned: volkswagenchick » Unassigned
Status: Needs work » Needs review
StatusFileSize
new11.32 KB

i have uploaded a patch that corrects some minor mistakes and converted the file to .md to utilize markdown.

volkswagenchick’s picture

Issue summary: View changes
jqtnguyen’s picture

Status: Needs review » Needs work

Attempted to apply patch on simplytest.me, but encountered an error which is likely on simplytest.me's side.

I found some other minor typos as well, and also have a few grammatical suggestions.

  1. +++ b/README.md
    @@ -0,0 +1,177 @@
    +possible to enable ajax for multipage groups. It also provides developers with
    ...
    +       the form pages are ajaxified there are no limitations.
    

    Capitalize "Ajax" for consistency.

  2. +++ b/README.md
    @@ -0,0 +1,177 @@
    +   https://www.drupal.org/project/field_group_ajaxified_multipage
    +
    ...
    +   https://www.drupal.org/project/issues/field_group_ajaxified_multipage
    

    Suggestion to add a comma after "By using a Field Group".

  3. +++ b/README.md
    @@ -0,0 +1,177 @@
    +multipage form, however, the paging can only be done on client side which has
    

    Suggestion to add a period after "multipage form" and start a new sentence with "However".

  4. +++ b/README.md
    @@ -0,0 +1,177 @@
    +    1. Validation of form fields is very basic, real validation is done after
    

    Suggestion to end sentence with "basic" and begin new sentence with "Real validation".

  5. +++ b/README.md
    @@ -0,0 +1,177 @@
    +       the form is submitted and often users will have to return to previous
    

    Suggestion to re-word "often users will" to "users will often".

  6. +++ b/README.md
    @@ -0,0 +1,177 @@
    +       the form pages are ajaxified there are no limitations.
    ...
    + * Install the Field Group Ajaxified Multipage module as you would normally
    

    Capitalize "Ajaxified" for consistency, and suggestion to add a comma afterward.

  7. +++ b/README.md
    @@ -0,0 +1,177 @@
    +    3. When the form is complex and has many pages with different fields it can
    

    Suggestion to add comma after "it", which will also result in a number of the following lines to need editing as well in order to stay within the 80 character per line limit.

  8. +++ b/README.md
    @@ -0,0 +1,177 @@
    +       become considerably heavy to load since by using only javascript for
    +       hiding/showing related fields to each page, the whole form needs to be
    

    Suggestion to rephrase as "can become considerably heavy to load since it uses javascript to hide/show related fields on each page, but the whole form is still loaded."

  9. +++ b/README.md
    @@ -0,0 +1,177 @@
    +       simply clone the next button added by this module and change it's title
    

    Typographical error. This should be "its", not "it's".

volkswagenchick’s picture

Assigned: Unassigned » volkswagenchick

Thanks for the review. I am going to re-assign myself to the issue and patch again tomorrow. Sumplytest.me appears to be broken, hopefully that will be remedied by then.

volkswagenchick’s picture

Assigned: volkswagenchick » Unassigned
Status: Needs work » Needs review
StatusFileSize
new10.95 KB

I addressed all of the concerns in comment #4 and uploaded a new patch. Simplytest.me was still not working when I uploaded this patch.

jqtnguyen’s picture

Status: Needs review » Needs work

I was able to apply the Patch, but unable to get the Module working using the instructions in the README.

+++ b/README.txt
-- Enable the module at Administer >> Modules.
+   https://www.drupal.org/project/field_group_ajaxified_multipage
...
+   https://www.drupal.org/project/issues/field_group_ajaxified_multipage
 
...
+    2. Setup your multipage field groups as usual for your entity/node.

It's very unclear what to do at this step for someone unfamiliar with the module or multipage.

volkswagenchick’s picture

Status: Needs work » Needs review
eduardo morales alberti’s picture

I checked the patches and I created mine.
The readme was a copy of drupal 7 version. It is now adapted to what is currently available.

Please review it and if it's right or there are better changes please tell me.

Thank you.

omarlopesino’s picture

Seems good the new README for 8.x. Great!

Just this correction in line 47:

Later enable the module.

->

Then enable the module.

eduardo morales alberti’s picture

Okey, I applied your changes.

Thank you.

eduardo morales alberti’s picture

Status: Needs review » Fixed
volkswagenchick’s picture

Thankyou for all the work you do!!
I am encouraging team mates to help out on Drupal.org, could you go back and give JQTNguyen a credit for his RTBC?? The box next to his name just gets ticked and he gets a credit! I see it as a way of incentive to help get involved it the community! It takes a village.

Cheers!

AmyJune - volkswagenchick

eduardo morales alberti’s picture

eduardo morales alberti’s picture

Okey volkswagenchick,
I already acredited in this issue to JQTNguyen

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.