Checklists: Redux state, declarative structure #26216

sirreal · 2018-07-20T13:32:56Z

Implementing ideas from #26015
Supersedes #26030
Closes #25958

This strikes a middle-ground of keeping array-of-objects that can be iterated over to produce a full checklist or a banner.

It also provides the option of building completely declarative checklists, which we do with the store checklist.

The props produced by connect are fewer and less likely to change.

Most importantly, we avoid merging declarative, structural information with state.

This will be helpful for future progress, such as #25935 and #25954

Todo:

✅ Remove obsolete blocks
✅ Remove obsolete helpers (mergeObjectIntoArrayById)
✅ Fix classNames
✅ Update devdocs
(postponed) Put launchTask somewhere better (Redux actions or middleware)
This could be handled in a follow-up, everything works in this PR, this is additional cleanup.

Testing

Checklists may be used in 4 places. The functionality should remain unchanged for all of them:

Functionality:

Task counts
Task ordering (completed at the top)
Checklist collapsing
Clicking the event actions
Reporting of event completion

Checklists in use to test:

Checklist banners (simple sites at /stats/day/SITE) (@taggon / @fditrapani)
Jetpack checklist (Jetpack sites at /plans/my-plan/SITE) (@Automattic/luna)
Store checklist (Store installed, visit Store dashboard at /store/SITE) (@timmyc)
Devdocs https://calypso.live/devdocs/design/checklist?branch=update/checklist-separate-state-structure (@sirreal)

Devdocs playground (@sirreal):
Drop the following in https://calypso.live/devdocs/playground?branch=update/checklist-separate-state-structure

<Checklist inferCompletedCount>
	<ChecklistTask
		onClick={ () => console.log('Do this?') }
		onDismiss={ () => console.log('Dismissing…') }
		title="Reticulate splines"
		buttonText="Reticulate!"
		buttonPrimary
		completedTitle="Splines are reticulated 👍"
		description="Make sure that all the splines are reticulated."
		duration="1 minute"
		completed
	/>
	<ChecklistTask
		onClick={ () => console.log('Do this?')  }
		onDismiss={ () => console.log('Dismiss yak shaving.') }
		title="Shave yaks!"
		buttonText="Buzzzz"
		buttonPrimary
		completedTitle="Yaks shaved."
		description="Make sure you shave the yaks so you can get on with your life."
		duration="10,000 minutes"
	/>
</Checklist>

cc: @ockham @fditrapani

matticbot · 2018-07-20T13:32:57Z

Test live: https://calypso.live/?branch=update/checklist-separate-state-structure

fditrapani · 2018-07-20T18:40:45Z

From a code perspective, this stuff is a little over my head. Let me know how I might be able to test it out when it's ready.

sirreal · 2018-07-20T21:44:21Z

client/blocks/checklist/task.js

+		 * This should be a simpler dispatched action.
+		 * Look at moving logic into middleware.
+		 */
+		void launchTask( {


This should go somewhere else and we'd just dispatch an action…

Yeah, I think ideally, I'd love to decouple the tour from the checklist even more -- i.e. really just pass in an onClick handler function, and make the Task block ignorant about GT specific stuff such as tourSlug, tourUrl, etc.

Follow-up material tho.

(Oh, which might mean we would possibly ditch the Task block and try to encapsulate that bound launchTask function elsewhere...)

Okay, scrolling down to the Task component, I realize this is the major distinction between the component and the block. I think I'd still even go as far as to possibly remove the block altogether if we can find a different way of encapsulating the launchTour function...

Yes, I agree with you here @ockham. I think this PR is a good balance of introducing the new, more declarative checklist components, and the Task block may well be a temporary measure.

My inclination is that this PR is in a sweet spot right now, it introduces the components and maintains a connected block that makes the state.checklist-connected Tasks trivial to declare.

Finding the right place to handle tour start from the task (and whether that's implemented where/how it should be, or if it should be rethought entirely) can be part of a follow up.

What are your thoughts?

Yep, sounds good! I think what seems the most arbitrary to me is that coupling of Guided Tours internals to the checklist. Agree that this PR is a good step towards decoupling that 👍

sirreal · 2018-07-20T21:50:43Z

client/components/checklist/header.js

@@ -40,6 +38,7 @@ export class ChecklistHeader extends PureComponent {
 					<ProgressBar compact total={ total } value={ completed } />
 				</div>
 				<div className="checklist__header-secondary">
+					{ /* eslint-disable-next-line jsx-a11y/label-has-for */ }


nesting and id are required, I'm not sure why. This file is moved but otherwise unchanged. Ignore rule.

sirreal · 2018-07-20T22:23:10Z

@fditrapani This is ready for some testing. I've added some instructions about where the Checklists can be found for testing and what should be expected (same behavior as staging/production). I'd appreciate any testing feedback you can provide, thanks!

timmyc

I didn't do a full review of the code, but this is testing out perfectly for the usage in Store.

ockham · 2018-08-03T20:32:47Z

client/components/checklist/README.md

+		buttonPrimary
+		completedTitle="Yaks shaved."
+		description="Make sure you shave the yaks so you can get on with your life."
+		duration="10,000 minutes"


sirreal · 2018-08-06T17:03:09Z

I've pinged a few folks for some manual testing as a checklist in the PR description.

If folks will contribute some manual testing to a checklist in their domain, @Automattic/luna and @ockham should be able to handle code review.

Thanks!

Cherry-picked from: d3d9ba1

Adds Components from #26216 Uses components for WooCommerce Store checklist Uses components for ChecklistShow Removes blocks (discourage block usage)

sirreal · 2018-08-14T13:07:45Z

Closing in favor of #26618 and follow-up work.

Adds Components from #26216 Uses components for WooCommerce Store checklist Uses components for ChecklistShow Removes blocks (discourage block usage)

* Add and use Checklist components, remove blocks Add components from #26216 Use components for WooCommerce Store checklist Use components for ChecklistShow Remove blocks Update devdocs accordingly * Separate state and structure in checklist banner

This PR continues separation of state and declarative structure work outlined in #26216. Inspired by suggestions from ockham, the Checklist is declared once in idiomatic React JSX, and can be rendered as a checklist of tasks or a banner (see `viewMode` prop). * `ChecklistShow`, responsible only for rendering the WordPress.com checklist, has been removed. * The array of task props in `onboardingChecklist` has been removed, and is now JSX in `WpcomChecklist`. * `ChecklistBanner` has been removed and replaced by `<WpcomChecklist viewMode="banner" />`. * `ChecklistMain` renders the `WpcomChecklist` and some surrounding UI. Clean up its usage of task status for calculating completion. * Remove the `mergeObjectIntoArrayById` util which is no longer needed and related tests

sirreal added [Status] In Progress [Type] Janitorial State Checklist [Type] Feature progress labels Jul 20, 2018

sirreal self-assigned this Jul 20, 2018

sirreal mentioned this pull request Jul 20, 2018

Checklists: Try declarative structure/redux state approach #26030

Closed

sirreal mentioned this pull request Jul 20, 2018

Checklists: State and structure #26015

Closed

sirreal force-pushed the update/checklist-separate-state-structure branch from 3a413a7 to beb61d9 Compare July 20, 2018 21:41

sirreal commented Jul 20, 2018

View reviewed changes

sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 20, 2018

sirreal force-pushed the update/checklist-separate-state-structure branch from a7d4a2d to 2f8355f Compare July 20, 2018 22:10

sirreal added the [Status] In Progress label Jul 20, 2018

sirreal requested review from timmyc, ockham and taggon July 20, 2018 22:17

sirreal mentioned this pull request Jul 20, 2018

ChecklistShow: Decouple tasks prop #25958

Closed

timmyc reviewed Jul 23, 2018

View reviewed changes

ockham force-pushed the update/checklist-separate-state-structure branch from 2f8355f to 1292d25 Compare August 3, 2018 20:13

ockham reviewed Aug 3, 2018

View reviewed changes

sirreal force-pushed the update/checklist-separate-state-structure branch from 1292d25 to f56f5cd Compare August 6, 2018 15:36

sirreal requested review from fditrapani and a team August 6, 2018 16:56

sirreal added 11 commits August 9, 2018 09:55

Use declarative checklists

396b9df

Use declarative checklist for Store

83edd4d

Cherry-picked from: d3d9ba1

Move style to component

f6e59a9

Update checklist devdocs

9c15475

Move remaining checklist related components

4300331

Remove unused checklist blocks

2fe42d7

Restore checklist-show placeholder

47f17ca

Clean up classes, styles, filenames

0a8742c

Remove unused utils

6b87114

Disable eslint a11y label-has-for rule

422fcd9

Affront the eslinter

1bc0cd9

sirreal force-pushed the update/checklist-separate-state-structure branch from d1b2e6f to 1bc0cd9 Compare August 9, 2018 13:56

sirreal mentioned this pull request Aug 10, 2018

Add and use Checklist components, remove blocks #26618

Merged

5 tasks

sirreal added a commit that referenced this pull request Aug 10, 2018

Add and use Checklist components, remove blocks

09cf00c

Adds Components from #26216 Uses components for WooCommerce Store checklist Uses components for ChecklistShow Removes blocks (discourage block usage)

sirreal added a commit that referenced this pull request Aug 13, 2018

Add and use Checklist components, remove blocks

acf9a1a

Adds Components from #26216 Uses components for WooCommerce Store checklist Uses components for ChecklistShow Removes blocks (discourage block usage)

sirreal closed this Aug 14, 2018

matticbot removed the [Status] In Progress label Aug 14, 2018

sirreal added a commit that referenced this pull request Aug 15, 2018

Add and use Checklist components, remove blocks

26e84e8

Adds Components from #26216 Uses components for WooCommerce Store checklist Uses components for ChecklistShow Removes blocks (discourage block usage)

sirreal deleted the update/checklist-separate-state-structure branch August 16, 2018 17:29

sirreal mentioned this pull request Aug 17, 2018

Checklist: Unify WordPress.com checklist and banner #26764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checklists: Redux state, declarative structure #26216

Checklists: Redux state, declarative structure #26216

sirreal commented Jul 20, 2018 •

edited

Loading

matticbot commented Jul 20, 2018

fditrapani commented Jul 20, 2018

sirreal Jul 20, 2018

ockham Aug 3, 2018 •

edited

Loading

ockham Aug 3, 2018 •

edited

Loading

ockham Aug 3, 2018

sirreal Aug 6, 2018

ockham Aug 7, 2018

sirreal Jul 20, 2018

sirreal commented Jul 20, 2018

timmyc left a comment

ockham Aug 3, 2018

sirreal commented Aug 6, 2018 •

edited

Loading

sirreal commented Aug 14, 2018

Checklists: Redux state, declarative structure #26216

Checklists: Redux state, declarative structure #26216

Conversation

sirreal commented Jul 20, 2018 • edited Loading

Testing

Checklists in use to test:

matticbot commented Jul 20, 2018

fditrapani commented Jul 20, 2018

sirreal Jul 20, 2018

Choose a reason for hiding this comment

ockham Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

ockham Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

ockham Aug 3, 2018

Choose a reason for hiding this comment

sirreal Aug 6, 2018

Choose a reason for hiding this comment

ockham Aug 7, 2018

Choose a reason for hiding this comment

sirreal Jul 20, 2018

Choose a reason for hiding this comment

sirreal commented Jul 20, 2018

timmyc left a comment

Choose a reason for hiding this comment

ockham Aug 3, 2018

Choose a reason for hiding this comment

sirreal commented Aug 6, 2018 • edited Loading

sirreal commented Aug 14, 2018

sirreal commented Jul 20, 2018 •

edited

Loading

ockham Aug 3, 2018 •

edited

Loading

ockham Aug 3, 2018 •

edited

Loading

sirreal commented Aug 6, 2018 •

edited

Loading