NEW Show icons on tabs with invalid form field values

[emteknetnz]

Enhancement for #1109

Youtube video demo

Used to communicate which tabs have invalid form field values

Design

Design elements implemented:

• Tab icons
• Red alert banner

Design elements not implemented:

• 'X' on the red alert banner to dismiss the banner
• New style for the invalid form field elements (red border, red text underneath)

TODO before peer review:

• merge new font icon and rebase this pull-request
• check accessibility on the tab icons

TODO testing

• Have agreed with @brynwhyman that we'll skip doing automated testing before merge on this one, simply because of the need to add frameworktest to the silverstripe-admin .travis.yml to get a custom page type for testing. We'll do this in a subsequent issue.
• Include details of the custom page type as a comment on this pull-request
• Write out the behat test as a comment

TODO before closing:

• Design review (Sacha said she'd like to do this after CSS has been peer reviewed)
• Push translation changes to transifix

[emteknetnz]

Test only multi-tab page type to be added to frameworktest in a subsequent pull-request and can be used for manual testing

I'm not sure how to automatically test the 2 validate() functions, particularly the $result->addError() scenario

There are 3 failed validation scenarios:

1. A required field in getCMSValidator() is blank in the CMS
2. In validate() $result->addFieldError is not comment out
3. In validate() $result->addError is not comment out

<?php

namespace {

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

    class MultiTabPage extends SiteTree
    {
        private static $db = [
            'SecondTabFirstField' => 'Varchar(50)',
            'ThirdTabFirstField' => 'Varchar(50)',
            'ThirdTabSecondField' => 'Varchar(50)',
            'FourthTabFirstField' => 'Varchar(50)',
            'SettingsTabFirstField' => 'Varchar(50)',
        ];

        private static $has_one = [];

        public function getCMSFields()
        {
            $fields = parent::getCMSFields();
            $fields->addFieldToTab("Root.Second", TextField::create("SecondTabFirstField"));
            $fields->addFieldToTab("Root.Third", TextField::create("ThirdTabFirstField"));
            $fields->addFieldToTab("Root.Third", TextField::create("ThirdTabSecondField"));
            $fields->addFieldToTab("Root.Fourth", TextField::create("FourthTabFirstField"));
            return $fields;
        }

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

        public function getCMSValidator()
        {
            return new RequiredFields([
                'ThirdTabFirstField',
                'FourthTabFirstField',
                'SettingsTabFirstField'
            ]);
        }

        public function validate()
        {
            $result = parent::validate();

            // validation error on speicific form field - uncomment to test
            $result->addFieldError('SecondTabFirstField', 'This field cannot exist.');

            // general validation error on form  - uncomment to test
            // try testing with `->addFieldError` line above commented out and not comment out
            $result->addError('This page cannot exist.');

            return $result;
        }
    }
}

[emteknetnz]

Behat test should look something like this. It should be in the silverstripe/silverstripe-cms repo which deals with page types, rather than here in silverstripe/silverstripe-admin

Will also need to create a function probably in FixtureContext.php for creating multi-tab pages. However given the pagetype should only exist in frameworktest, it gets quite messy

We should discuss before implementation if we even want to do this at all.

Feature: Multi-tab page validation icons
  As a content author
  I want to see which tabs have form fields that failed validation
  So that I know what I need to fix before saving

  Background:
    Given a "multi-tab page" "My MultiTab Page"

  Scenario: I can see tab validation icons
    Given I go to "/my-multitab-page"
    When I press the "Save" button
    Then I should not see an invalid tab icon on "Second tab"
    Then I should see an invalid tab icon on "Third tab"
    Then I should see an invalid tab icon on "Fourth tab"
    When I click on "Third tab"
    And I fill in "Third tab first field" with "abc"
    When I press the "Save" button
    Then I should not see an invalid tab icon on "Second tab"
    Then I should not see an invalid tab icon on "Third tab"
    Then I should see an invalid tab icon on "Fourth tab"
    When I click on "Third tab"
    And I fill in "Fourth tab first field" with "def"
    When I press the "Save" button
    Then I should not see an invalid tab icon on "Second tab"
    Then I should not see an invalid tab icon on "Third tab"
    Then I should not see an invalid tab icon on "Fourth tab"

[Cheddam]

Left some feedback on the implementation; also found another edgecase - if a DataObject::validate() method triggers addFieldError() but not addError(), none of the new behaviour triggers.

I've also noticed that addMessage and addFieldMessage don't render at all, but this seems to be an existing bug, so we can raise a separate issue for that.

[Cheddam]

Rather than using flex, I suggest taking this icon out of the standard flow, as the current approach pushes away the tab to the right.

Suggested change

	align-self: flex-start;
	margin-top: 12px;
	margin-left: -4px;
	position: relative;
	top: 12px;
	right: 4px;

(You should also be able to drop out the other flex rules above.)

[emteknetnz]

I think the horizontally moving tabs is actually intentional, otherwise you end up with not much whitespace between tabs

position: absolute

flex

@sachajudd ?

[sachajudd]

I peer reviewed this yesterday with @Cheddam and we would like to keep the spacing between tabs consistent as seen per designs. I didn't notice this in our initial design review together.

[emteknetnz]

Updated

[Cheddam]

A couple more minor items - once these are addressed (along with the existing styling feedback) we should be good to merge.

[sachajudd]

Approved from a UX POV

[dnsl48]

lgtm