Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This simple module uses the Field Group api to create a formatter that delimits multiple fields. This is distinct from Field Delimiter as this acts across multiple fields in a field group, not on single, multiple cardinality fields.
https://drupal.org/sandbox/johnbburg/2163075
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/johnbburg/2163075.git
Thanks in advance.
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #2
impol CreditAttribution: impol commentedModule looks clean.
Please change "git clone" section on issue summary to "git clone --branch 7.x-1.x http://git.drupal.org/sandbox/johnbburg/2163075.git field_group_delimiter", because yours link needs authentication by yours account (johnbburg@git.drupal.org).
Comment #3
mraichelson CreditAttribution: mraichelson commentedPending the git url mentioned above this looks good and does what it says on the box.
Comment #4
jneubert CreditAttribution: jneubert commentedThe module installed cleanly with it's dependency.
The code looks good, with sensible inline comments (minor typo: "deliimits").
However, I'm not sure how the module is supposed to work. I've before defined custom formats for inline display of multiple items with templates, where I could put the delimiters as required "in code".
With field_group_delimiter, I could add a new group, put two fields "dummy1" and "dummy2" into it, and add a "//" delimiter. It shows up as
I'm sure I missed something, but I found no way to turn this into an inline display.
Comment #5
bburgThank you for your input jneubert. This is an excellent point. I originally developed with ds_extras installed and using very specific setup under the "Expert" field layout option. Clearly this module will need some work to make it cooperate better with default field templates.
Comment #6
bburgComment #7
bburgComment #8
rafenden CreditAttribution: rafenden commentedHi, what's the difference between this module and Field group Inline?
Comment #9
bburgThanks Rafal. I Thought it seemed odd that I couldn't find a module that did this, but Field group inline seems to to exactly what this module set out to do. So I will close this ticket.
Comment #10
dcmouyard CreditAttribution: dcmouyard commentedThe Field Group Inline module adds styles that force elements within it to be inline. This sandbox module just adds the delimiter, which is my preferred method.
Comment #11
mraichelson CreditAttribution: mraichelson commented@dcmouyard: That may be your preferred method but if the submitter of this project isn't interested in continuing the application process and maintaining this moving forward we can't force him to. Since he's the one that closed it, it should probably stay closed (unless johnbburg decides to pursue it again).
Comment #12
bburg@mraichelson, Your note is valid, but dcmouyard and I work with each other, and he is encouraging me to go through with this module submission, and I am indeed interested in doing so, regardless of the alternative Field Group Inline.
Comment #13
bburgI have implemented several updates to this module.
As for the duplicative nature of this module. As dcmouyard mentioned, he, as a themer, finds this module very useful as it provides an option to insert markup between fields without being forced into an inline style.
Comment #14
BigEd CreditAttribution: BigEd commentedHi johnbburg,
Coder Sniffer has found some issues with your code
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 1 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
92 | ERROR | Expected "foreach (...) {\n"; found "foreach(...) {\n"
93 | WARNING | Line exceeds 80 characters; contains 106 characters
94 | ERROR | Expected "if (...) {\n"; found "if(...) {\n"
95 | ERROR | Concat operator must be surrounded by spaces
95 | ERROR | Concat operator must be surrounded by spaces
96 | ERROR | Array indentation error, expected 10 spaces but found 12
97 | ERROR | Array closing indentation error, expected 8 spaces but found
| | 10
101 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
--------------------------------------------------------------------------------
Looking into this in more depth later.
Comment #15
BigEd CreditAttribution: BigEd commentedComment #16
bburgThanks for the review, I had run an earlier version through the sniffer, but had also implemented a massive refactor after some suggestions from a co-worker. I'll fix those just as soon as I can.
Comment #17
BigEd CreditAttribution: BigEd commentedI had a look through the module code it looks quite clean and straight forward so not much to comment on.
The only thing I would add is that on line 93 the comment is longer that the 80 chars.
I think its good to go.
Comment #18
PA robot CreditAttribution: PA robot commentedClosing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).
I'm a robot and this is an automated message from Project Applications Scraper.