Issue #3497000: Add SEO settings section to page data form.

marked this merge request as ready

We currently don't have any tests that add a Page entity via the form, correct? Just wondering what the general state of the Page entity is. Usually would expect to see tests proving the new form fields actually update the entity on save

requested changes

added 3 commits

• 18d36048 - 1 commit from branch project:0.x
• 83be622e - Add SEO settings section to page data form.
• f1303f3e - Create explicit form for xb_page.

Compare with previous version

requested review from @mglaman

I thought we could add #group to the element and that would move it without reassigning the variable around.

I tried with this, but weights were not getting assigned properly due to which order in which the elements appear inside seo settings was incorrect.

I'm okay with this approach then if #group wasn't working, for the sake of getting it in versus a rabbit hole of debugging.

I agree with @mglaman's original remark.

If NodeForm::form() can do

    if (isset($form['uid'])) {
      $form['uid']['#group'] = 'author';
    }

Then this should be able to do something similar.

If the order is not as desired, then only overriding #weight should be sufficient?

Adding #group attribute and then overriding #weight didn't work. It might have worked in Drupal UI but since we are rendering it using react, those weights are not getting picked up. Weights only got picked up when I manually assigned the elements inside seo_settings.

Then I think @bnjmnm should review this — this sounds like a bug in https://www.drupal.org/project/issues/experience_builder?component=Redux-integrated+field+widgets. This work-around should not be necessary. I'm not opposed to it landing here, but I am opposed to it landing without a @todo pointing to a follow-up issue.

I brought down the branch and while #group worked fine, I also encountered the #weight values not being respected for image and description... Regardless of the theme used to render it. A fairly quick way to rule out React rendering is to temporarily change this line in the Theme Negotiator to something other than xb_stark and it will use that theme to render, Twig and everything.

I switched to Stark and the ordering still ignored #weight for image and description (description is always before image) but I can successfully move the title around with $form['metatags']['widget'][0]['basic']['title']['#weight'] to appear before or after the image/description grouping as needed.

It makes sense that this is not due to React as that rendering is no more responsible for ordering by weight than Twig is. While something is clearly amiss I'd like us to be careful attributing things to this part of XB with term bug even if it's speculative. The newness of it all is intimidating enough IMO.

Rather than force future XB maintainers to figure all this pain out again, added a comment pointing to @bnjmnm's excellent comment: 61d34a0d

Can we have an e2e test which edits a page and verifies this organization as well?

requested changes

added 24 commits

• f1303f3e...f2057353 - 21 commits from branch project:0.x
• 49a4a4e5 - Add SEO settings section to page data form.
• d288a8f6 - Create explicit form for xb_page.
• 77b6f62d - Add e2e test confirming the new seo section.

Compare with previous version

approved this merge request

requested review from @tedbow

requested review from @wimleers and @f.mazeikis

requested review from @balintbrews