Blocks: Polish and update timeline block to support alternating on large screens #41474

jasmussen · 2020-04-27T11:12:51Z

This PR affects the Timeline block. Before:

After:

Changes:

Simpler more theme-friendly design
New icon
Alternating timeline entries left and right at beyond 640px breakpoints

matticbot · 2020-04-27T11:12:53Z

Test live: https://calypso.live/?branch=update/timeline-block-v2

matticbot · 2020-04-27T11:17:05Z

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D42481-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

matticbot · 2020-04-27T11:24:33Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

mkaz · 2020-04-27T17:17:37Z

This tests well, no problems with new or existing timelines. One thing to note is it changes past timelines to the alternating format on upgrade, without any user intervention. So this might be surprising depending on how users have used the timelines in the past, for example as entries in a resume.

What about a setting for alternating entries? I know decisions and not options, but considering it will change existing content, this might be a better alternative.

A suggestion with changing the alternating entries, should we center the "Add new entry" inserter button, it feels a little odd having the button on the far left and then an item is added to thr right.

For clarification, what do you think about changing it to "Add new timeline entry"

jasmussen · 2020-04-28T07:19:17Z

One thing to note is it changes past timelines to the alternating format on upgrade, without any user intervention. So this might be surprising depending on how users have used the timelines in the past, for example as entries in a resume.

This is a good point. Can we do this?

There's a new toggle, global to the timeline block: "alternate bubbles on large screens".
This toggle is on by default for new timeline blocks. But off for existing timeline blocks.
This toggle, when on, simply adds a CSS class to the timeline block, is-alternating.

And if yes we can do this, can you help me create that toggle plus class for this block? I'll then leverage those CSS classes to change so that these things alternate per that class, not just nth-child. By the way, I have my own take on decisions not options ;)

A suggestion with changing the alternating entries, should we center the "Add new entry" inserter button, it feels a little odd having the button on the far left and then an item is added to the right.

I'll marinate on that one. I'm not immediately a fan of adding complexity here. For now this timeline update simply alternates left or right, or not at all. In a future update we might allow you to choose the side, rather than blanket alternate, at that point it would make total sense.

For clarification, what do you think about changing it to "Add new timeline entry"

Actually, I think we should remove that custom appender and replace it with the real appender. Because the issue that made us create a custom appender in the first place is no longer. Behold:

This is the issue: WordPress/gutenberg#16659 — and that will actually make it look like this:

Which at least is consistent with the other appenders, which I'd argue is more important than any extra labels. Is this also something you can help with? :D

jasmussen · 2020-06-05T07:19:32Z

I spoke with @georgeh yesterday about how to best proceed with this one. The challenge is that existing timeline blocks do not alternate, and it migth be wrong if suddenly they start to.

Which means the alternation should be a toggle — it can (should, IMO) be on by default for new blocks — but it can remain off for existing blocks, or you can toggle it off.

This to me suggests that as a baseline, there should be a single global toggle, on by default but off for existing blocks, that sits in the sidebar of the selected timeline block.

The next question then is: should you be able to choose on which side the timeline item shows up on? I.e. maybe you'd like 3 small items on the left, and one large item on the right? This would certainly be nice.

Could we do this?

When the global toggle is enabled, we alternate all child items using nth-child|odd/even as we do now. Odd on the left, even on the right.
We we have additional CSS classes — is-left and is-right, with the same CSS as we have for odd/even.
Individual timeline items have a radio group, or combobox control in the sidebar with three options: auto | left | right.

If we could do this, we could have a good alternating default experiene, with further customizability by the user. I would even suggest we do the explicit is-left and is-right toggle for individual items as a followup pull request, rather than as part of this PR.

georgeh · 2020-06-12T18:59:09Z

@jasmussen I've pushed up a couple commits with an is-alternating toggle for the whole Timeline, and is-left/is-right class choosers for each item.

jasmussen · 2020-06-12T19:02:12Z

Oh my goodness, so wonderful.

Can we make it a little bit like the alignment drop-down for images? No option chosen means auto, and then also left and right options?

georgeh · 2020-06-12T19:08:39Z

Yeah, that would simplify things a little, I'll push up a change

georgeh · 2020-06-12T19:20:12Z

Something like this, where you click the active one to de-activate?

jasmussen · 2020-06-12T19:36:18Z

Exactly like that, yes, but with the icons from image alignments. In fact there's room in the toolbar, no need to put it into a drop-down! So cool

jasmussen · 2020-06-15T09:15:41Z

Improve alignment controls for Timeline item blocks

This is awesome George, this is what I see:

It looks like the alternating is off by default. Can we make it on by default, but keep it off for existing blocks? Perhaps if we inversed the logic of the classname, so that instead of an is-alternating, we had a is-one-column, then when this class gets toggled off, that's when we alternate?

I'll push some CSS to polish the left/right alignment buttons:

I can also hide them entirely unless the timeline allows alternating. That will get us to the home stretch.

But I think we should move the alternation toggle to the sidebar entirely. So instead of this:

we do this:

☝️ this assumes the inverted logic I suggested above. But the idea is: you'll almost always want the alternation because it's fun. And if you need to toggle it off, it feels fine to put that in the sidebar.

jasmussen · 2020-06-15T09:36:13Z

Pushed polish to the buttons, made the explicit settings work front and backend:

I think we may need a deprecation handler as this adds a class to the frontend. Not sure, @georgeh can you tell?

jasmussen · 2020-06-15T09:38:51Z

Changed appender. AGain. To look like this:

jasmussen · 2020-06-15T09:44:40Z

@georgeh thanks for your help. I got this mostly working. I tried polishing the overall toggle where it is, and inverting the logic, and I got this far:

But I think I'm doing it wrong, and I could use your help. And also, if we are to invert the logic, we probably do need to show it in the sidebar so there's room for a little help text.

georgeh · 2020-06-22T15:17:13Z

@jasmussen Can I help by making the button label update and moving it to the sidebar? Or are you looking to change something else?

a8ci18n · 2020-06-22T15:37:33Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+								onClick={ () => toggleAlignment( 'left' ) }
+								isActive={ attributes.alignment === 'left' }
+								icon={ positionLeft }
+								title={ __( 'Left', 'full-site-editing' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 162 times:
translate( 'Left' ) ES Score: 10

a8ci18n · 2020-06-22T15:37:34Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+								onClick={ () => toggleAlignment( 'right' ) }
+								isActive={ attributes.alignment === 'right' }
+								icon={ positionRight }
+								title={ __( 'Right', 'full-site-editing' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 162 times:
translate( 'Right' ) ES Score: 10

a8ci18n · 2020-06-22T15:37:34Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+					<li style={ style } className={ classes }>
+						<InspectorControls>
+							<PanelColorSettings
+								title={ __( 'Color Settings', 'full-site-editing' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 100 times:
translate( 'Color Settings' ) ES Score: 10

a8ci18n · 2020-06-22T15:37:35Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+									{
+										value: attributes.background,
+										onChange: ( background ) => setAttributes( { background } ),
+										label: __( 'Background Color', 'full-site-editing' ),


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 153 times:
translate( 'Background Color' ) ES Score: 10
See 1 additional suggestions in the PR translation status page

jasmussen · 2020-06-22T15:38:21Z

Can I help by making the button label update and moving it to the sidebar? Or are you looking to change something else?

Yep, if we move that one to the sidebar it would be great, it allows it to be a toggle like the dropcap control, and it also hides it a bit because I feel like you'll almost always want this on by default.

The challenge, which is why I'm talking about inverting the logic and such, is that ideally anyone who creates a new Timeline block after we merge this PR, get the alternating feature by default, whereas any existing block created before this PR, have the alternating off. I don't know if this is possible.

Speaking of — can we/should we hide the explicit left/right alignment buttons for each timeline item, if the parent block has alternating set to off?

georgeh · 2020-06-23T20:29:22Z

What I'm reading is:

Legacy Block: should not alternate by default
New Block: should alternate by default

That makes a lot of sense: expose the functionality as soon as someone tries it out, but don't change anything that has already been created.

Sadly, I'm having trouble giving a different behavior for legacy blocks (no alternating attribute) and new blocks. If I specify a default attribute of isAlternating: true then legacy blocks get that. If I don't specify a default, then new blocks get undefined the same as legacy blocks. I'll keep poking around at ways to differentiate them.

jasmussen · 2020-06-24T06:12:38Z

Sadly, I'm having trouble giving a different behavior for legacy blocks (no alternating attribute) and new blocks. If I specify a default attribute of isAlternating: true then legacy blocks get that. If I don't specify a default, then new blocks get undefined the same as legacy blocks. I'll keep poking around at ways to differentiate them.

Ugh indeed, I figured it might be difficult. @mkaz do you have any insights as to how we can most easily ship this improvement without spending oceans of time on it? I imagine the easiest paths forward are these two:

Default to not alternating. Done.
Default to alternating, and affect every existing block, and be bullish about this behavior, maybe create a support doc detailing how to turn it off.

Does that sound right? I'm happy with either of the two, tbh, just in the name of shipping.

One final question, if you'll forgive me. Imagine the existing saved timeline markup for all created blocks was this:

<div class="timeline">
	<div class="timeline-item">
		...
	</div>
	<div class="timeline-item">
		...
	</div>
</div>

Now let's imagine the new markup was this:

<div class="timeline" data-alternating="true">
	<div class="timeline-item">
		...
	</div>
	<div class="timeline-item">
		...
	</div>
</div>

So any new blocks inserted would get the above, and alternate, but any exiting blocks without the data-alternating="true" prop would not. Would this require a deprecation handler since the explicitly off-toggled version would presumably have data-alternating="false"? And would this be feasible, pseudo code aside?

georgeh · 2020-06-24T13:16:09Z

I think you might be on to something with using the data-alternating attribute. I'm going to see if I can craft a selector to check if the attribute exists, which we could use to change the default behavior.

If that doesn't work, I vote we default to not alternating to avoid changing existing blocks.

a8ci18n · 2020-06-24T21:25:28Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+					onClick={ () => toggleAlignment( 'left' ) }
+					isActive={ alignment === 'left' }
+					icon={ positionLeft }
+					title={ __( 'Left', 'full-site-editing' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 162 times:
translate( 'Left' ) ES Score: 10

a8ci18n · 2020-06-24T21:25:29Z

apps/full-site-editing/full-site-editing-plugin/jetpack-timeline/blocks/src/timeline-item.js

+					onClick={ () => toggleAlignment( 'right' ) }
+					isActive={ alignment === 'right' }
+					icon={ positionRight }
+					title={ __( 'Right', 'full-site-editing' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 162 times:
translate( 'Right' ) ES Score: 10

georgeh · 2020-06-25T19:17:11Z

@jasmussen I moved the alternating toggle to the sidebar, please take a look and let me know what you think

jasmussen · 2020-06-29T08:11:02Z

Alright, I pushed some polish. Thank you George, I think this is ready for final review.

It could receive more love, there's a block appender I'd like to revisit, and if this sets the world on fire it could be interesting to revisit the default state. But as it is, this is a safe, and substantial update to the block:

This ports work from a previous PR.

…ault to not alternating

jasmussen · 2020-07-09T10:48:55Z

We reviewed this yesterday in our chat, and after discussing the best process, it is to merge this, deploy it, and let the block update go out with the next FSE version.

a8ci18n · 2020-07-23T14:21:10Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/4124310

Thank you @jasmussen for including a screenshot in the description! This is really helpful for our translators.

a8ci18n · 2020-07-30T16:33:11Z

Translation for this Pull Request has now been finished.

jasmussen added the Blocks Editor blocks, aka Gutenberg blocks, plugins, and extensions label Apr 27, 2020

jasmussen requested a review from mkaz April 27, 2020 11:12

jasmussen self-assigned this Apr 27, 2020

jasmussen changed the title ~~Blocks: Polish and update timeline block to support alterating on large screens~~ Blocks: Polish and update timeline block to support alternating on large screens Apr 27, 2020

jasmussen requested a review from a team as a code owner June 12, 2020 20:05

a8ci18n reviewed Jun 22, 2020

View reviewed changes

a8ci18n reviewed Jun 24, 2020

View reviewed changes

jasmussen and others added 15 commits July 2, 2020 15:10

Initial porting work.

6b1a129

This ports work from a previous PR.

Interim refactor work.

d615354

Polish appender.

1f4bd42

New icon.

406e903

Add alternating toggle to Timeline block

1c6535b

Add is-left and is-right class toggles for Timeline items

3882a79

Improve alignment controls for Timeline item blocks

48d33d4

Output left/right on frontend, fix styling, polish buttons.

3a0a021

Polish appender and rename again.

677cfc3

Increase specificity.

edf323d

Add is-alternating selector for styles

54678f0

Hide alignment buttons on TimelineItem blocks if not alternating, def…

4399cc2

…ault to not alternating

Move Alternating control to sidebar

5a7ad97

Polish alternating toggle.

d399d77

Polish alternating toggle

240aa1a

georgeh force-pushed the update/timeline-block-v2 branch from b7c1fda to 240aa1a Compare July 2, 2020 19:11

jasmussen merged commit 93d5425 into master Jul 9, 2020

jasmussen deleted the update/timeline-block-v2 branch July 9, 2020 10:49

deBhal mentioned this pull request Jul 13, 2020

Update FSE plugin to version to 1.13 #44125

Merged

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Polish and update timeline block to support alternating on large screens #41474

Blocks: Polish and update timeline block to support alternating on large screens #41474

jasmussen commented Apr 27, 2020

matticbot commented Apr 27, 2020

matticbot commented Apr 27, 2020

matticbot commented Apr 27, 2020

mkaz commented Apr 27, 2020

jasmussen commented Apr 28, 2020

jasmussen commented Jun 5, 2020

georgeh commented Jun 12, 2020

jasmussen commented Jun 12, 2020

georgeh commented Jun 12, 2020

georgeh commented Jun 12, 2020

jasmussen commented Jun 12, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

georgeh commented Jun 22, 2020

a8ci18n Jun 22, 2020

a8ci18n Jun 22, 2020

a8ci18n Jun 22, 2020

a8ci18n Jun 22, 2020

jasmussen commented Jun 22, 2020

georgeh commented Jun 23, 2020

jasmussen commented Jun 24, 2020

georgeh commented Jun 24, 2020

a8ci18n Jun 24, 2020

a8ci18n Jun 24, 2020

georgeh commented Jun 25, 2020

jasmussen commented Jun 29, 2020

jasmussen commented Jul 9, 2020

a8ci18n commented Jul 23, 2020

a8ci18n commented Jul 30, 2020

Blocks: Polish and update timeline block to support alternating on large screens #41474

Blocks: Polish and update timeline block to support alternating on large screens #41474

Conversation

jasmussen commented Apr 27, 2020

matticbot commented Apr 27, 2020

matticbot commented Apr 27, 2020

matticbot commented Apr 27, 2020

mkaz commented Apr 27, 2020

jasmussen commented Apr 28, 2020

jasmussen commented Jun 5, 2020

georgeh commented Jun 12, 2020

jasmussen commented Jun 12, 2020

georgeh commented Jun 12, 2020

georgeh commented Jun 12, 2020

jasmussen commented Jun 12, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

jasmussen commented Jun 15, 2020

georgeh commented Jun 22, 2020

a8ci18n Jun 22, 2020

Choose a reason for hiding this comment

a8ci18n Jun 22, 2020

Choose a reason for hiding this comment

a8ci18n Jun 22, 2020

Choose a reason for hiding this comment

a8ci18n Jun 22, 2020

Choose a reason for hiding this comment

jasmussen commented Jun 22, 2020

georgeh commented Jun 23, 2020

jasmussen commented Jun 24, 2020

georgeh commented Jun 24, 2020

a8ci18n Jun 24, 2020

Choose a reason for hiding this comment

a8ci18n Jun 24, 2020

Choose a reason for hiding this comment

georgeh commented Jun 25, 2020

jasmussen commented Jun 29, 2020

jasmussen commented Jul 9, 2020

a8ci18n commented Jul 23, 2020

a8ci18n commented Jul 30, 2020