Remove hardcoding #group for the status field

Can you please test this with what we're working on in #3356717: Move Action buttons to sticky header?

@maks

tested it on the dev version with the suggested changes from #3356717: Move Action buttons to sticky header
nothing has changed

Okay thanks for verifying

(I also noticed that the sidebar switch is hidden in the ellipsis, it seems it shouldn't be like that)

That is intentional

We have experienced this same issue in farmOS for some time where the entity status field is not a checkbox. I solved it in other ways (our own version of GinContentForm so to say) but agree with the proposed fix to only move the status when it is a checkbox.

Furthermore, I'm running into this issue on *simple forms* (not entity forms, not gin content forms) after #3356717: Move Action buttons to sticky header. With this change any status field in a form is moved out of the form to the actions area. Not only is this unexpected, but it also results in this status field not being included in the form submission (I believe because the status is moved without the additional template changes that content forms receive?)

I think the fix is two parts:
1. Only move the status field on gin content forms
2. Only move the status field when it is a checkbox

I've opened a new MR with these changes. It also includes some changes to simplify GinContentFormHelper logic and also likely increase performance (don't invoke thegin_content_form_routes hook multiple times!!!)

I've rebased this MR onto the latest 8.x-3.x, there was only one minor conflict with the last commit with this MR. Still need a review on this

Not able to Replicate this issue. With drupal version(11.0.x) and gin theme version(8.x-3.x-dev), Checked with installing examples module (https://www.drupal.org/project/examples) also, But not able to reproduce it. Let me know if other specific configuration is needed, thanks!

Checked with installing examples module (https://www.drupal.org/project/examples) also, But not able to reproduce it. Let me know if other specific configuration is needed, thanks!

Hi @maninders, appreciate you trying to recreate this. The important thing is that the form you are testing has a form field called status. I'm not sure if there is a custom form in the Examples module that fits this criteria. Testing likely requires creating a custom form.

I tested with the latest 8.x-3.x and confirmed the same issue is happening. There were some changes in GinContentForm so I've rebased my MR 446 onto the latest 8.x-3.x. Again, the full diff for this MR is a little ugly, please review the 4 commits individually to better understand the changes. I changed the order of these commits to make them easier to review.

The first two commits fix the issues previously described in this issue.

The third commit in the MR has the most changes because it combines two if statements into one and un-indents code from a nested if statement to simplify relevant logic in GinContentHelper::formAlter. These are the two if statements before changes:

    // Sticky action buttons.
    if ($this->stickyActionButtons($form, $form_state, $form_id) || $this->isContentForm($form, $form_state, $form_id)) {
      // Action buttons.
      if (isset($form['actions'])) {

In the last commit, instead of calling isContentForm() and invoking `hook_gin_content_for_routes` 3 separate times we can save the result in a variable and reference this value as needed.

Ultimately, the logic in the two above if statements are simplified to:

if (($use_sticky_action_buttons || $is_content_form) && isset($form['actions'])) {

I mentioned in Slack that I might open a separate issue with these additional changes that are not necessary to fix the issue. But because these changes would either conflict with or be dependent on the fix in this MR, it is a much easier contribution to include them here. If maintainers still consider this out of scope then please only consider the first two commits from my MR. Cheers

Yes, agreed. While it is literally off-topic, it really makes sense to optimize this code altogether. I've looked through and I'm happy with it, so setting to RTBC.

@paul121 maybe we should also open a follow-up issue to optimize \Drupal\gin\GinContentFormHelper::isContentForm as we should probably cache the result of that fairly expensive method.

maybe we should also open a follow-up issue to optimize \Drupal\gin\GinContentFormHelper::isContentForm as we should probably cache the result of that fairly expensive method.

Oh interesting! I haven't done something like this before. Curious how you would go about that.

Thanks Paul for your work on this 👏

Hello,

Just updated Gin.
Looks like the value of
$widget_type = $form['status']['widget']['#type'] ?? FALSE; is alsways false in my case.

Because the #type to check is in $form['status']['widget']['value']['#type'] and not in $form['status']['widget']['#type'].

And so the status checkbox doesn't go into the gin sticky actions / status container.

Am I alone ?

@flocondetoile you're correct. I missed that as I had content moderation turned on where we don't have that checkbox at all. I'll propose a fix in a minute.

Please give the new MR a try.

Looks good and fix the issue

Thanks @flocondetoile 👏