Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Focus on first tab with field that failed validation #1108

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Sep 17, 2020

Update: we're unlikely to merge this, because even though it's a regression from previous behaviour, we'll probably enhance this and put alert icons on tabs with invalid form fields, instead of tab switching


Partial fix for silverstripe/silverstripe-cms#2081

Based on Robbie noticing this silverstripe/silverstripe-cms#2081 (comment)

Seems like this is a regression of functionality that used to work. At some point we must have change the classes used for tabs and validation messages, as well as possibly how tabs are structured. Likely while going from SS3 to SS4.

Testing setup:

Page.php

<?php

namespace {

    use SilverStripe\CMS\Model\SiteTree;
    use SilverStripe\Forms\RequiredFields;
    use SilverStripe\Forms\TextField;

    class Page extends SiteTree
    {
        private static $db = [
            'ABC' => 'Varchar',
            'DEF' => 'Varchar',
            'XYZ' => 'Varchar',
            'S22' => 'Varchar',
        ];

        private static $has_one = [];

        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldToTab("Root.Second", TextField::create("ABC"));

            // used to test javascript scroll into view
            $fields->addFieldToTab("Root.Third",TextField::create("null1"));
            $fields->addFieldToTab("Root.Third",TextField::create("null2"));
            $fields->addFieldToTab("Root.Third",TextField::create("null3"));
            $fields->addFieldToTab("Root.Third",TextField::create("null4"));
            $fields->addFieldToTab("Root.Third",TextField::create("null5"));
            $fields->addFieldToTab("Root.Third",TextField::create("null6"));
            $fields->addFieldToTab("Root.Third",TextField::create("null7"));
            $fields->addFieldToTab("Root.Third",TextField::create("null8"));
            $fields->addFieldToTab("Root.Third",TextField::create("null9"));
            $fields->addFieldToTab("Root.Third",TextField::create("null10"));
            $fields->addFieldToTab("Root.Third",TextField::create("null11"));
            $fields->addFieldToTab("Root.Third",TextField::create("null12"));
            $fields->addFieldToTab("Root.Third",TextField::create("null13"));
            $fields->addFieldToTab("Root.Third",TextField::create("null14"));

            $fields->addFieldToTab("Root.Third", TextField::create("DEF"));
            
            $fields->addFieldToTab("Root.Fourth", TextField::create("XYZ"));
            return $fields;
        }

        public function getSettingsFields()
        {
            $fields = parent::getSettingsFields();
            $fields->addFieldToTab("Root.Settings", TextField::create('S22'));
            return $fields;
        }

        public function getCMSValidator() {
            return new RequiredFields(array('DEF', 'XYZ', 'S22'));
        }
    }
}
  1. Click Save without filling in any fields
  2. It should focus on the Tab "Third" and you should see a "DEF" is required validation message.
  3. Fill the DEF field in, click save
  4. You should now be focused on the Tab "Fourth" and see a "XYZ" is required validation message.

@emteknetnz
Copy link
Member Author

emteknetnz commented Sep 17, 2020

I'm not sure how we'd go about solve the issue originally in issue, which was related to validating errors in different top level tabs ("Content" | "Settings" | "History") - in the example code about there is new field in settings S22 which can be 'left blank' while in the "Content" tab, and the Page will still save correctly. Only when in the "Settings" tab and clicking "Save" will validation get triggered for S22

What this fix does do is fix this for the next tab level down validation issues - there was an issue on an internal project with tabs at the lower level under the top level "Content" tab: i.e. "Root.Main" | "Root.Second"

@emteknetnz
Copy link
Member Author

emteknetnz commented Sep 17, 2020

Behat failure looks to be an existing failure on admin 1.6

@emteknetnz emteknetnz force-pushed the pulls/1.6/focus-on-invalid-tab branch from 769c4d6 to ca8eebe Compare September 17, 2020 03:31
@emteknetnz emteknetnz marked this pull request as draft September 17, 2020 04:12
@brynwhyman brynwhyman mentioned this pull request Sep 17, 2020
4 tasks
@emteknetnz emteknetnz closed this Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant