Skip to content

Conversation

@andersonjeccel
Copy link
Contributor

Q A
Bug fix? (use the a.b branch) 🔴
New feature/enhancement? (use the a.x branch) 🟢
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🔴
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

Switches are going to replace the Yes/No buttons, keeping the same backend way to handle data saving to ensure compatibility with absolutely all existing functionalities and JS code.

image

This PR also features a mini toggle where space is an issue, when you're adding them to tables or Reports filter rows.

image

Have a look at this example (not a real implementation, just made it look small to show). The "Use theme style" field is using a small toggle when compared to the others.

image

The beauty here is the UX improvement, as it allows to have custom labels according to the field status. THey help to understand the consequences of each state. Look at the status labels for the Kiosk mode:

Disabled Enabled
image image

In many cases, they can replace tooltips.
These custom labels are set using the existing PHP functionality. We have a class for the Yes/No buttons that provides a default value for no_label and yes_label (below):

class YesNoButtonGroupType extends AbstractType
{
    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults(
            [
                'choices'           => fn (Options $options): array => [
                    $options['no_label']  => $options['no_value'],
                    $options['yes_label'] => $options['yes_value'],
                ],
                'choice_value'      => function ($choiceKey) {
                    if (null === $choiceKey || '' === $choiceKey) {
                        return null;
                    }

                    return (is_string($choiceKey) && !is_numeric($choiceKey)) ? $choiceKey : (int) $choiceKey;
                },
                'expanded'          => true,
                'multiple'          => false,
                'label_attr'        => ['class' => 'control-label'],
                'label'             => 'mautic.core.form.active',
                'placeholder'       => false,
                'required'          => false,
                'no_label'          => 'mautic.core.form.no',     // labels here
                'no_value'          => 0,
                'yes_label'         => 'mautic.core.form.yes',
                'yes_value'         => 1,
            ]
        );
    }
}

The custom labels can be set when adding fields to the builder, providing a new translation string for each:

        $builder->add('inKioskMode', YesNoButtonGroupType::class, [
            'label'     => 'mautic.form.form.kioskmode',
            'attr'      => [
                'tooltip' => 'mautic.form.form.kioskmode.tooltip',
            ],
            'yes_label' => 'mautic.form.form.kioskmode.yes',
            'no_label'  => 'mautic.form.form.kioskmode.no',
        ]);

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Go to Forms > New > Toggle the option Kiosk mode
  3. Go to Segments > New > Toggle Visible for other users > Check the label for No (Only me)
  4. Go to Points > Point Actions > New > Toggle Is repeatable? and see two different labels
  5. Go to Reports > New > Toggle Visible for all logged-in users to see the No label
  6. Go to Custom Fields > New > Toggle the Indexable field
  7. Go to Custom fields > New > Check a new and improved tooltip for Unique identifier

Go to Forms > New > Toggle the option Kiosk mode
Go to Segments > New > Toggle Visible for other users > Check the label for No (Only me)
Go to Points > Point Actions > New > Toggle Is repeatable? and see two different labels
Go to Reports > New > Toggle Visible for all logged-in users to see the No label
Go to Custom Fields > New > Toggle the Indexable field
Go to Custom fields > New > Check tooltip for Unique identifier
@andersonjeccel andersonjeccel self-assigned this Sep 25, 2024
@andersonjeccel andersonjeccel added T1 Low difficulty to fix (issue) or test (PR) user-interface Anything related to appearance, layout, and interactivity ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging user-experience Anything related to related to workflows, feedback, and navigation enhancement Any improvement to an existing feature or functionality php Pull requests that update Php code labels Sep 25, 2024
@andersonjeccel
Copy link
Contributor Author

@LordRembo I investigated a bit more of my history, we had an accessible version in this commit using the button idea

the issue is that it would cause an absurd effort to fix all JS functions that depends on this radio structure used over the years and might not generate a stable result (e.g. we might miss some functionalities along the way that will just break for the end user)

I'm avoiding this kind of change tbh, but I can give access to my fork if you see an easy way to do it
My search used "checked" for the entire codebase, limiting to .js files (if you'd like to have a look)

Checked these:

Tested adding to Twig

onclick="Mautic.toggleYesNo(this);"
               role="switch"
               aria-checked="{{ value == yes_choice.value ? 'true' : 'false' }}"
               tabindex="0"

and this to JS, but it'd need further validation

...
$label.attr('aria-checked', 'false');
    } else {
        // Switch to 'Yes'
        $yesInput.prop('checked', true).trigger('change');
        $noInput.prop('checked', false);
        $switchEl.addClass('toggle__switch--checked');
        $textEl.text(yesText);
        $label.attr('aria-checked', 'true');
    }
};

Mautic.handleKeyDown = function(event, element) {
    if (event.key === ' ' || event.key === 'Enter') {
        event.preventDefault();
        Mautic.toggleYesNo(element);
    }
};

What do you think?

@andersonjeccel andersonjeccel removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Sep 30, 2024
@andersonjeccel
Copy link
Contributor Author

@abhisekmazumdar new test failing here

@abhisekmazumdar abhisekmazumdar self-assigned this Oct 2, 2024
@escopecz
Copy link
Member

escopecz commented Oct 2, 2024

@andersonjeccel I fixed the tests in #14157. Please review.

@andersonjeccel
Copy link
Contributor Author

@abhisekmazumdar so probably no fix needed here anymore

@andersonjeccel andersonjeccel added code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged hacktoberfest Issues that would be great for Hacktoberfest participants to work on labels Oct 2, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

I did basic tests, and works for me.

@andersonjeccel andersonjeccel added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged code-review-passed PRs which have passed code review user-testing-passed PRs which have been successfully tested by the required number of people. and removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged hacktoberfest Issues that would be great for Hacktoberfest participants to work on labels Oct 3, 2024
@RCheesley RCheesley merged commit 9cc7180 into mautic:5.x Oct 3, 2024
@andersonjeccel andersonjeccel deleted the ux-toggle branch October 3, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-review-passed PRs which have passed code review enhancement Any improvement to an existing feature or functionality php Pull requests that update Php code ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR) twig user-experience Anything related to related to workflows, feedback, and navigation user-interface Anything related to appearance, layout, and interactivity user-testing-passed PRs which have been successfully tested by the required number of people.

Projects

Archived in project
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants