I've been testing the module over the past few days and one of the big usability things I noticed is required fields within groups. Currently if someone is wanting to quickly create a node of some sort, you really have to click through every group to make sure you don't miss a required field. I understand this is very low on priority but I created a patch that adds a recursive asterisk as a proof of concept. I do think it can be done better than my patch does it, but it's a start.

On a side note, I've noticed a lot of problems with the vertical group items automatically going into the additional settings group on the node add/edit form. I've had zero success making a vertical group completely separate. I've also had issues anytime a group(of any type) is nested inside of another group. The demonstration site on the module page shows this working when the node is viewed, however i've been unable to get it to work on my local copy of d7-1. If you are aware of this problem then I'll check out any other issues on it and help find an answer. If you aren't, I'll create a topic and gather some more info.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

I like the asterix idea, nice and clean! Wondering if this maybe should also be a setting in the backend or not ?

Stalski’s picture

Status: Needs work » Active

I will check out the patch today.

For the other problem, there was a problem i notices with existing groups together with newly added group (since rc4 to drupal core release). I removed my groups and added them again, and the problem was gone.

The vertical tabs issue is something i am aware of. In the documenation i explain the fact that on node forms you could ommit the vertical tabs wrapper if you want to merge your vertical tabs with the ones in the additional settings section.
However, it should be possible to add a new wrapper and separate your new set of tabs. I will test this again as you explain it is broken.

michaellander’s picture

Swentel, I'm glad you like the idea! One of my concerns however with my approach is that other modules currently won't be following the same protocol of adding an asterisk to the label. So for example ubercart, as it was in drupal 6, had required sku and price fields that were in a vertical tab on the additional settings page. Those are sort of out of our control in regards to having an asterisk.

Stalski, I did try readding all the groups, but they seem to eventually corrupt themselves if that makes sense. I'm also not sure I've ever had success with nesting one group inside of another on the node add/edit form. I did notice the part in the documentation about adding the tabs to additional settings, however it appears that is working a little too well as it seems to be grabbing every vertical tab including the ones in vertical tab groups ha!

Anyways, I am going to set up a fresh install locally and run some tests and I'll grab some screenshots. If the problems don't exist on the clean install then it may be easier for me to locate the issue.

As for my patch, it's a pretty rough start, particularly I think we may be able to improve the way it iterates through the array to check if required. Granted, it works, but it just seems sloppy to me(I'm still learning). The other part that I'm unsure how to fix is that the title fields automatically get cleaned so I'm unable to retain the span tags around the asterisk upon render.

Anyways, thanks for the feedback and I'll come back with more info.

Stalski’s picture

Great :)
I am doing some tests as well.

michaellander’s picture

Screenshots! Hopefully I'm not just configuring them wrong haha.

You can see where the problems start occurring when groups are nested. The one common problem across all types of nested elements is the ghost element that appears below the parent group. So it basically duplicates on of the tab names and just displays it below.

The accordion inside of horizontal doesn't have too many problems, but the horizontal group inside of accordion has a ton. Also, as you are already aware, you can see the vertical group tabs just always appear in additional settings.

I'm going to start going through it myself and see if I can track down the bugs. Do you want me to create a new issue since it is independent from the asterisk patch?

Stalski’s picture

Status: Active » Needs review

I did change a bit of things and your patch did not handle fields with und][0][ . Fixed that too.
I made a configuration page with apropriate permission on it configure the behavior of vertical and horizontal tabs.
http://drupal.org/cvs?commit=477480

Stalski’s picture

Yes, create another issue please for the things that have nothing to do with the asterisk.

I can tell you that i did a big change as well since i saw something was broken big time. I tell you this so you maybe update to DRUPAL-7--1 again so you work with last code.

I think that only horizontal and vertical tabs should have that behavior. However, you might say, why not accordion as well. So maybe that should be configurable per type.

Some further thoughts?

Stalski’s picture

TODO: fix the recursive required group mark for images. That does not seem to work yet.

michaellander’s picture

I moved the Vertical Group stuff to here: #1020116: Confusing verbage across group types. It turned out to be some user error but I think the verbiage can be improved.

You can ignore the off topic stuff on this issue and I'll do some more testing with the newest code and I'll resubmit the group nesting bugs(or any other bugs)as new issues to try and make your life easier.

Update: It appears all the node add/edit nesting issues were fixed with this latest update. I had to go back and readd all the groups again and then I tested every possible combo and they worked great. Good work! There are a few css issues, I'll troubleshoot those next.

Stalski’s picture

Great, thx for the testing and review :)

michaellander’s picture

Stalski’s picture

thx, i will test it out asap.
But it's bizar, i really thought that was encountered in the else beneath where i loop through the children. I think i need to look deeper so the generic approach really is generic.

michaellander’s picture

That foreach statement is checking:
$element[$child]['und']['#required']

instead of

$element[$child]['und'][0]['#required']

The foreach is iterating the 'und' currently. We could put another foreach in there or we can just assume that if it's required, at 0 would have it. I can't think of an instance where index 1 would be required and index 0 wouldn't, but I'm still new to this.

One other possible solution would be to just search through the entire element to find the #required tag and then get the result. I could only see this be useful if there is a lot of variation between field types.

Stalski’s picture

Status: Needs review » Fixed

Committed and tested by at least two people

Dave Reid’s picture

So, apparently I missed this as this functionality wasn't working for us. I ported the solution that is in the Vertical tabs module issue queue for Drupal 6 (since core vertical tabs in D7 don't provide this functionality yet - hopefully we can get it into D7.1) that uses a simple JS solution. Just a five-line solution in #1035626: Add a required indicator for horizontal tab elements that contain a required field, so maybe it would be easier to implement that rather than the PHP solution? Especially since if the user has JS disabled, there's no point in doing a server-side solution since all the user will see is an un-collapsed fieldset.

Stalski’s picture

Status: Fixed » Needs work

As talked about in IRC, good point.
The PHP code is not pretty while the client approach is easy. To Be Refactored

Stalski’s picture

Status: Needs work » Closed (duplicate)