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

Change btn-primary class of email buttons in campaign send email action to btn-default #10837

Merged

Conversation

volha-pivavarchyk
Copy link
Contributor

@volha-pivavarchyk volha-pivavarchyk commented Feb 7, 2022

Q A
Bug fix? (use the a.b branch) [No]
New feature/enhancement? (use the a.x branch) [Yes]
Deprecations? [No]
BC breaks? (use the c.x branch) [No]
Automated tests included? [No]
Issue(s) addressed Fixes #...

Description:

The 3 buttons on the send email action are using the class btn-default instead of btn-primary

The current view:
image

The suggestion:
image

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a new campaign
  3. Launch campaign builder
  4. Add contact segment
  5. Add send email action
  6. Email buttons have the btn-default class
    image

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Feb 7, 2022
@RCheesley
Copy link
Sponsor Member

Thanks for the PR @volha-pivavarchyk - is there a reason why you think that we should not make the buttons visually stand out using the current CSS class?

@RCheesley RCheesley added email Anything related to email enhancement Any improvement to an existing feature or functionality user-experience Anything related to related to workflows, feedback, and navigation labels Feb 8, 2022
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #10837 (8eb375c) into 4.x (1ef4462) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                4.x   #10837   +/-   ##
=========================================
  Coverage     47.32%   47.32%           
  Complexity    35044    35044           
=========================================
  Files          2104     2104           
  Lines        117731   117731           
=========================================
  Hits          55719    55719           
  Misses        62012    62012           
Impacted Files Coverage Δ
...pp/bundles/EmailBundle/Form/Type/EmailSendType.php 0.00% <0.00%> (ø)

@kuzmany
Copy link
Member

kuzmany commented Feb 8, 2022

I do not see benefits by this change.

@adiux
Copy link
Contributor

adiux commented Feb 8, 2022

@RCheesley From a UX perspective, there should always be one button that is the "primary". One button the user should click to go to the next step.

With the current code we have 3 buttons as primary, and none of them is the default action that takes the user to the next step. That would be the "add button".

@RCheesley
Copy link
Sponsor Member

That would be the "add button".

That depends, really doesn't it? If you are not selecting an email from the dropdown, your next step is to create an email.

I do see what you mean about the Add button being the main 'next step' button in the grand scheme of things, I wonder if these are using the primary class to make them visually stand out among the other fields.

@adiux
Copy link
Contributor

adiux commented Feb 8, 2022

@RCheesley of course this depends. If this is a greenfield I would take another approach.

The way I look at it right now the button that gets the most clicks should be the primary. To me, this is the add button.

But to change the save button is a bit more complex. It would have an impact on many forms (at least I think so). I think it is defined here for many buttons: app/bundles/CoreBundle/Form/Type/FormButtonsType.php
btn btn-default btn-save => btn btn-primary btn-save

So I suggest merging this, and looking at the other issue for a major release.

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.

After this comment #10837 (comment) and check visual again, I am okay with change and like it.
Less primary buttons on modal window is better for me

@kuzmany kuzmany added the pending-test-confirmation PR's that require one test before they can be merged label Feb 9, 2022
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

That sounds like a good approach @adiux and thanks for the explanation - I feel like we may have a UX standardisation project on the not-too-distant horizon!

@kuzmany kuzmany added this to the 4.2 milestone Feb 9, 2022
@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 9, 2022
@kuzmany kuzmany merged commit 10141af into mautic:4.x Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The PR contributors have signed the contributors agreement email Anything related to email enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged user-experience Anything related to related to workflows, feedback, and navigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants