Mailchimp Block: UI Review Updates #30685

jeffersonrabb · 2019-02-09T18:45:27Z

Changes proposed in this Pull Request

This PR contains a series of UI and design improvements flagged by @thomasguillot and @scruffian in #30667. Since the block uses Server-Side Rendering, many of these fixes involve similar changes in the Jetpack repo. Here's the list of changes. Quoted text is from the issue.

Editor UI:

I don't see the need for us to have a heading as part of the block. If for some reason we need one, we should have the same options as the Heading block.

Done in 141d9f2 and Automattic/jetpack@9431ca9

Can the email input be a type="email" rather than type="text"

Done in e363a8c and Automattic/jetpack@d921ac3

Could we have a button like the Button block and remove the label option from the sidebar?

Done in da91702.

Notes/Questions:

I applied a class from the core Button block ( wp-block-button__link ) to get some of the style without duplicating a bunch of CSS. This might be a questionable practice since future changes Core Button could affect Mailchimp in unexpected ways. Curious if there are opinions on the best way to handle this situation.
Core Button's background color defaults to #0073aa, which is not one of the Muriel blues. It seemed preferable to replicate the Core style rather than stick to Muriel colors and create dissonance, but I'm happy to go either way on this.
On the Frontend, the button has only these classes components-button is-button is-primary, which should leave the button style to the theme.

The text underneath, could it just be a Paragraph block (within the block) with some italic applied to it? At the moment, the italic setting doesn't work since it's already in italic. I'm not quite sure why this is a figcaption either, from an HTML point of view this makes no sense and it's invalid.

Done in 7f61c70 and Automattic/jetpack@f801919. I applied a few additional styles here:

wp-calypso/client/gutenberg/extensions/mailchimp/view.scss

Lines 32 to 35 in 7f61c70

    
           .wp-block-jetpack-mailchimp_consent-text { 
        
           	color: $gray; 
        
           	font-size: 0.8em; 
        
           	font-style: italic;

Some places it's written "MailChimp", and some other "Mailchimp". The correct version is "Mailchimp" now.

Done in caaf827 and Automattic/jetpack@1a0be76.

Note: In the WP.com sharing interface, there are still instances of MailChimp that should be changed. @artpi would you be willing to take care of these when you have a chance?

Front-end:

The forced styles on the email input are not needed .wp-block-jetpack-mailchimp .wp-block-jetpack-mailchimp-email themes style their inputs and forcing this is not recommended.

Done in 49e51ba and Automattic/jetpack@5dba739

Same with .wp-block-jetpack-mailchimp .wp-block-jetpack-mailchimp-form-error I would suggest we add a generic class error to the input. Again, most themes will style this.

Done in 4653bca.

Note: This is tough to test now that the input field is type email, since the browser automatically enforces validation. Also, in the Inspector I tried adding the generic error class to the input, and didn't see any visual change.

Displaying all 3 messages in the content with a display: none feels strange. Can we just add the message with some JS?

For the Editor, this is done in c8d0bf7.

Note: For the Frontend, it isn't terribly straightforward. The approach would probably be to store the notification text bits as data attributes on the block element, and then rework view.js to put the appropriate text in the DIV. I'd be happy to do this, but it feels like swapping one form of awkwardness for another, so I'm not sure there's much of a win here. If people feel that this should be done, I'll take it on in a separate PR.

For all the color-related changes, it wasn't entirely clear to me where the discussion in the issue landed. Muriel? Gutenberg? For now, I just used @thomasguillot's original colors as-is. If the consensus was to go a different way, let me know and I'll change.

Processing, have a more subtle background: rgba(0, 0, 0, 0.025)

Done in d1e4518

Success: Let's use an accessible green #1d7819

Done in d1e4518

Same for error: #eb0001

Done in d1e4518

.wp-block-jetpack-mailchimp .wp-block-jetpack-mailchimp-notification let's switch to ems: margin-bottom: 1.5em and padding: 0.75em.

Done in 38e9cd2

Remove the text-center from the messages. If the user adds a bunch of text to these messages, it wouldn't look great centred.

Done in ce80348

Testing instructions

https://jurassic.ninja/create/?gutenpack&calypsobranch=update/mailchimp-ui-changes&branch=update/mailchimp-ui-changes
Enable Jetpack
Enable JETPACK_BETA_BLOCKS in Settings->Jetpack Constants
Add Mailchimp, verify all visual/UI changes.

Fixes #30667

…y styling.

…nes.

…moval of some unnecessary classes on the frontend, and changes to the view script to reflect these removals. Some style definitions moved from view stylesheet to editor's since they aren't used on frontend.

…n the editor.

matticbot · 2019-02-09T18:45:28Z

Test live: https://calypso.live/?branch=update/mailchimp-ui-changes

thomasguillot · 2019-02-11T10:06:17Z

It looks great, thank you @jeffersonrabb! I just have a couple of notes/remarks:

Core Button's background color defaults to #0073aa, which is not one of the Muriel blues.

Core Button's background color is good 👍 I'm just wondering now if we should allow for background/text custom colors like the actual Button block: https://cloudup.com/coHwOau93wT

Also, we shouldn't have a "link" option for the button and italic/bold doesn't seem to work, I think it's safe to remove these: https://cloudup.com/czHUFkuNB1N

I applied a few additional styles here

Rather than forcing the color, font-size and font-style, can we just have a  text and then leave the rest to the user/theme to decide?

sirreal

A quick comment, I'll have another look when design feedback is addressed.

sirreal · 2019-02-11T11:06:57Z

client/gutenberg/extensions/mailchimp/edit.jsx

-						<TextControl placeholder={ emailPlaceholder } onChange={ () => false } />
-						<Button isPrimary>{ submitLabel }</Button>
+						<TextControl placeholder={ emailPlaceholder } onChange={ () => false } type="email" />
+						<div className="wp-block-button__link wp-block-jetpack-mailchimp_button">


Was it impossible to use the Button component here? I'd very much prefer to rely on that component rather than a class it uses.

If we need the styling, I think we're better off pulling the necessary styles into our own CSS.

The goal was to emulate the Gutenberg button block in appearance and functionality - and since https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/button/edit.js doesn't make use of the Button component I fear that this would lead to slight style divergences and then CSS to overcome them. Re: the styling, yes I'll pull it in.

If we need the styling, I think we're better off pulling the necessary styles into our own CSS.

This is done in 8f066fd

jeffersonrabb · 2019-02-11T11:18:30Z

Also, we shouldn't have a "link" option for the button and italic/bold doesn't seem to work, I think it's safe to remove these: https://cloudup.com/czHUFkuNB1N

Removed all the formatting controls in bc6db32

jeffersonrabb · 2019-02-11T11:20:37Z

Rather than forcing the color, font-size and font-style, can we just have a  text and then leave the rest to the user/theme to decide?

Confirming, the suggestion is to replace the  tag with ? (as opposed to adding  as a wrapper around it?)

…onsistency with Core Button block without relying on a class from that block (wp-block-button__link).

jeffersonrabb · 2019-02-11T12:02:26Z

I'm just wondering now if we should allow for background/text custom colors like the actual Button block: https://cloudup.com/coHwOau93wT

I like the idea, but fear this puts us on a slippery slope of chasing the core button block. If we allow color customization would we also need to support core Button's style choices (Rounded|Outline|Squared)? Button has what looks like fairly dense color fallback handling, and uses a component called ContrastChecker that seems a little ... daunting. If changes were made to the Button block down the road presumably we'd want to replicate them in Mailchimp. Finally, the front-end rendering of the button in Mailchimp is quite a bit simpler than it is for Button block, using a <button> element with a couple of classes on it effectively leaving the button styling to the theme. If we introduce visual customization elements for the button colors this would need to be examined.

Given all of this, if you still feel we ought to pursue color options for the button I'd propose we land the current PR and address in a new one. @thomasguillot what do you think?

thomasguillot · 2019-02-11T12:26:04Z

Rather than forcing the color, font-size and font-style, can we just have a  text and then leave the rest to the user/theme to decide?

Confirming, the suggestion is to replace the  tag with ? (as opposed to adding  as a wrapper around it?)

........

thomasguillot · 2019-02-11T12:27:27Z

Given all of this, if you still feel we ought to pursue color options for the button I'd propose we land the current PR and address in a new one. @thomasguillot what do you think?

Fair enough, thank you for the explanation @jeffersonrabb. Let's keep it as is 😊

jeffersonrabb · 2019-02-11T12:35:11Z

........

I'm not sure this structure is possible - we're using the RichText component which allows setting the containing element as an attribute (

wp-calypso/client/gutenberg/extensions/mailchimp/edit.jsx

Line 200 in 8f066fd

tagName="p"

). However, there isn't support that I'm aware of for structured tag hierarchy within the element. If I simply change the tagName attribute to small, the output looks pretty good but there's no paragraph tag. Is this invalid HTML? Also note this applies only to the editor. In the front-end we can structure the output however we'd like.

sirreal · 2019-02-11T12:47:02Z

I'm not sure this structure is possible

Can't you do the following?

<p><RichText tagName="small" /><p>

jeffersonrabb · 2019-02-11T12:52:38Z

Can't you do the following? <RichText tagName="small" />

RichText inserts a bunch of container <div>s, so if we take this approach we end up with markup like this:

@thomasguillot is this acceptable markup?

thomasguillot · 2019-02-11T13:01:00Z

@thomasguillot is this acceptable markup?

It's not 😞p can't contain div. It's ok. For the editor we can just go with small and have p smallfor the front-end.

sirreal · 2019-02-11T13:06:29Z

RichText inserts a bunch of container <div>s

😞 I actually came across similar problem when I tried to use it recently. I wonder if it could be improved to help with cases like this. There do seem to be short-term plans to iterate on the component: WordPress/gutenberg#13778

…declaration.

jeffersonrabb · 2019-02-11T13:39:00Z

It's not 😞p can't contain div. It's ok. For the editor we can just go with small and have p smallfor the front-end.

So much 😞around this one! Changes made in d59edb6 and Automattic/jetpack@f4a7d65. I left the color and italics declarations in but removed the sizing - is this correct?

thomasguillot · 2019-02-11T13:55:19Z

It's not 😞p can't contain div. It's ok. For the editor we can just go with small and have p smallfor the front-end.

So much 😞around this one! Changes made in d59edb6 and Automattic/jetpack@f4a7d65. I left the color and italics declarations in but removed the sizing - is this correct?

Can we just remove the entire CSS for .wp-block-jetpack-mailchimp_consent-text?

small is enough to differentiate the text from the rest of the content.

jeffersonrabb · 2019-02-11T14:48:32Z

Can we just remove the entire CSS for .wp-block-jetpack-mailchimp_consent-text?

Done in 1bc9f82 and Automattic/jetpack@6961def#diff-e800fe1ba24cea2c2da6f6ac9f542d4dR68

thomasguillot · 2019-02-11T14:56:56Z

Front-end doesn't seem to work anymore

jeffersonrabb · 2019-02-11T15:06:07Z

Front-end doesn't seem to work anymore

Try pulling the Jetpack branch (if you're testing locally), or create a new JN instance. Just resolved the issue in Automattic/jetpack@89d53f0.

thomasguillot

LGTM

client/gutenberg/extensions/mailchimp/view.scss

…line toolbar, to allow selection of the submit button after consent text editing has taken place. Use appropriate cursor for button text hover and editing.

jeffersonrabb · 2019-02-11T17:28:28Z

@artpi pointed out an issue in which the consent text's inline toolbar blocks clicks on the submit button, making the button non-editable. This is fixed along with a change to the button text's cursor in 4518d4e.

@thomasguillot does this approach raise any red flags?

thomasguillot · 2019-02-11T17:35:16Z

@thomasguillot does this approach raise any red flags?

Nope, all good 🙂

jeffersonrabb added 12 commits February 9, 2019 10:11

UI review: remove title from Mailchimp block.

141d9f2

UI Review: change input field type to "email".

e363a8c

UI Review: For consent text use P tag instead of FIGCAPTION, and appl…

7f61c70

…y styling.

UI Review: Consistent capitalization for Mailchimp, per brand guideli…

caaf827

…nes.

UI Review: Editable submit button, patterned after core Button block.

da91702

UI Review: CSS class cleanup. Classname naming convention changes. Re…

49e51ba

…moval of some unnecessary classes on the frontend, and changes to the view script to reflect these removals. Some style definitions moved from view stylesheet to editor's since they aren't used on frontend.

UI Review: use generic 'error' class instead of block-specific one.

4653bca

UI Review: Colors for notification boxes.

d1e4518

UI Review: Notification box colors.

c8c7893

UI Review: EMs for notification box padding and margins.

38e9cd2

UI Review: Remove centered text alignment on notification boxes.

ce80348

UI Review: Alternate approach to auditioning the notification boxes i…

c8d0bf7

…n the editor.

jeffersonrabb mentioned this pull request Feb 9, 2019

Mailchimp Block: UI Review Updates Automattic/jetpack#11317

Merged

jeffersonrabb added Mailchimp [Goal] Gutenberg Working towards full integration with Gutenberg [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 9, 2019

jeffersonrabb requested review from artpi, thomasguillot, scruffian and sirreal February 9, 2019 19:20

jeffersonrabb self-assigned this Feb 9, 2019

UI Review: Remove formatting controls for editable button text.

bc6db32

sirreal reviewed Feb 11, 2019

View reviewed changes

Duplicated styles from Gutenberg repo for button, to maintain style c…

8f066fd

…onsistency with Core Button block without relying on a class from that block (wp-block-button__link).

Render Mailchimp consent text with a SMALL element, remove font-size …

d59edb6

…declaration.

Remove consent text class and styling. Using SMALL tag is sufficient.

1bc9f82

thomasguillot approved these changes Feb 11, 2019

View reviewed changes

simison reviewed Feb 11, 2019

View reviewed changes

client/gutenberg/extensions/mailchimp/view.scss Outdated Show resolved Hide resolved

simison reviewed Feb 11, 2019

View reviewed changes

client/gutenberg/extensions/mailchimp/view.scss Outdated Show resolved Hide resolved

jeffersonrabb added 3 commits February 11, 2019 11:52

Use matching Muriel color for Mailchimp error notification.

5f34885

Muriel green for success notification.

83bc6ab

Allow click events to pass through the background of the rich text in…

4518d4e

…line toolbar, to allow selection of the submit button after consent text editing has taken place. Use appropriate cursor for button text hover and editing.

jeffersonrabb merged commit 4e9adc3 into master Feb 11, 2019

jeffersonrabb deleted the update/mailchimp-ui-changes branch February 11, 2019 18:02

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mailchimp Block: UI Review Updates #30685

Mailchimp Block: UI Review Updates #30685

jeffersonrabb commented Feb 9, 2019 •

edited

Loading

matticbot commented Feb 9, 2019

thomasguillot commented Feb 11, 2019

sirreal left a comment

sirreal Feb 11, 2019

jeffersonrabb Feb 11, 2019

jeffersonrabb Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

sirreal commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

sirreal commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot left a comment

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

	.wp-block-jetpack-mailchimp_consent-text {
	color: $gray;
	font-size: 0.8em;
	font-style: italic;

Mailchimp Block: UI Review Updates #30685

Mailchimp Block: UI Review Updates #30685

Conversation

jeffersonrabb commented Feb 9, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Feb 9, 2019

thomasguillot commented Feb 11, 2019

sirreal left a comment

Choose a reason for hiding this comment

sirreal Feb 11, 2019

Choose a reason for hiding this comment

jeffersonrabb Feb 11, 2019

Choose a reason for hiding this comment

jeffersonrabb Feb 11, 2019

Choose a reason for hiding this comment

jeffersonrabb commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

sirreal commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

sirreal commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 11, 2019

thomasguillot left a comment

Choose a reason for hiding this comment

jeffersonrabb commented Feb 11, 2019

thomasguillot commented Feb 11, 2019

jeffersonrabb commented Feb 9, 2019 •

edited

Loading