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

EZP-32212: Changed class to correct recognition ezmatrix fieldType #1662

Merged
merged 4 commits into from
Dec 10, 2020

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Dec 8, 2020

Question Answer
Tickets EZP-32212
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Related PR: ezsystems/ezplatform-matrix-fieldtype#45
Passed tests: https://travis-ci.com/github/ezsystems/ezplatform-page-builder/builds/207258337

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@mateuszdebinski mateuszdebinski changed the title [WIP]EZP-32212: Changed class to correct recognition ezmatrix fieldType EZP-32212: Changed class to correct recognition ezmatrix fieldType Dec 8, 2020
@mateuszdebinski mateuszdebinski marked this pull request as ready for review December 8, 2020 15:04
@mateuszdebinski mateuszdebinski self-assigned this Dec 8, 2020
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Why is this diff different from the current state of this file in 1.5 branch?
https://github.com/ezsystems/ezplatform-admin-ui/blob/1.5/src/lib/Behat/PageElement/ContentField.php#L46-L63
There's a condition missing for ezuser.

Ezmatrix tests will now share the same selector with ezmatrix and the one for ezmatrix will be more generic, so the condition for ezuser should be first, something like this should be ok:

    private function getFieldTypeIdentifier(string $fieldClass): string
    {
        if ($fieldClass === '') {
            return 'ezboolean';
        }

        if ($fieldClass === 'ez-scrollable-table-wrapper mb-0') {
            return 'ezuser';
        }

        if (strpos($fieldClass, 'ez-scrollable-table-wrapper') !== false) {
            return 'ezmatrix';
        }

        preg_match($this::FIELD_TYPE_CLASS_REGEX, $fieldClass, $matches);
        $matchedValue = explode('-', $matches[0])[0];

        return $matchedValue;
    }

@mateuszdebinski
Copy link
Contributor Author

Q: Why is this diff different from the current state of this file in 1.5 branch?
https://github.com/ezsystems/ezplatform-admin-ui/blob/1.5/src/lib/Behat/PageElement/ContentField.php#L46-L63
There's a condition missing for ezuser.

Ezmatrix tests will now share the same selector with ezmatrix and the one for ezmatrix will be more generic, so the condition for ezuser should be first, something like this should be ok:

    private function getFieldTypeIdentifier(string $fieldClass): string
    {
        if ($fieldClass === '') {
            return 'ezboolean';
        }

        if ($fieldClass === 'ez-scrollable-table-wrapper mb-0') {
            return 'ezuser';
        }

        if (strpos($fieldClass, 'ez-scrollable-table-wrapper') !== false) {
            return 'ezmatrix';
        }

        preg_match($this::FIELD_TYPE_CLASS_REGEX, $fieldClass, $matches);
        $matchedValue = explode('-', $matches[0])[0];

        return $matchedValue;
    }

corrected, I have forgotten git pull before creating this branch

Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@adamwojs adamwojs merged commit 10fc13a into 1.5 Dec 10, 2020
@adamwojs adamwojs deleted the EZP-32212-ezmatrix-overflow-table-behat-test branch December 10, 2020 08:48
@adamwojs
Copy link
Member

Could you please merge up changes @mateuszdebinski ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants