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.

Sign up for GitHub

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

Jump to bottom

Checklist: Unify WordPress.com checklist and banner #26764

Merged
merged 14 commits into from
Sep 10, 2018
Merged

Checklist: Unify WordPress.com checklist and banner #26764

merged 14 commits into from
Sep 10, 2018

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Aug 17, 2018 •
edited
Loading

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 sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. NUX [Type] Janitorial Checklist labels Aug 17, 2018
@sirreal sirreal self-assigned this Aug 17, 2018
@matticbot
Copy link
Contributor

matticbot commented Aug 17, 2018 •
edited by sirreal
Loading

Test live: https://calypso.live/checklist?branch=update/cleanup-checklist-main-show

@sirreal sirreal changed the title Checklist: Cleanup and unify WordPress.com NUX checklist Checklist: Unify WordPress.com checklist and banner Aug 17, 2018
@sirreal sirreal force-pushed the update/cleanup-checklist-main-show branch 3 times, most recently from f3027cf to e20eb5f Compare August 17, 2018 19:45
@sirreal sirreal requested review from ockham and taggon August 17, 2018 19:49
@sirreal sirreal force-pushed the update/cleanup-checklist-main-show branch from 298628a to da105ac Compare August 17, 2018 20:38
sirreal
sirreal commented Aug 17, 2018
View reviewed changes
Copy link
Member Author

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Explored different approach in da105ac

I'd love some feedback if anyone has great ideas here.

client/my-sites/checklist/main.jsx Outdated
'contact_page_updated',
'post_published',
'custom_domain_registered',
].includes( taskId ) || task.completed === true
Copy link
Member Author

@sirreal sirreal Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh… this defeats the whole purpose.

Server returns more Tasks than are in the Checklist. We don't want to report incomplete when we aren't rendering some server-reported tasks (we can't complete them!)

@sirreal
Copy link
Member Author

sirreal commented Aug 18, 2018 •
edited
Loading

Documenting that I considered only querying the tasks we're interested in, but this is a bit fragile and has its own issues.

Where does filtering happen? API does not currently support it, data-layer fromApi would be a good place to do it, but doesn't have access to the action and this creates coupling between fromApi and the component. Additionally, Jetpack and WPCOM checklists have different Tasks. It could be sent as part of the action, but fromApi does not have access to the action so some special handling would need to be implemented.

ockham
ockham reviewed Aug 20, 2018
View reviewed changes
client/components/checklist/index.js Outdated
const count = Children.map( children, child => child.props.completed ).reduce(
( acc, completed ) => ( true === completed ? acc + 1 : acc ),
const completedCount = Children.toArray( children ).reduce(
( count, task ) => ( true === task.props.completed ? count + 1 : count ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use countBy() instead of reduce() here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might, but I'm not familiar with countBy. If you want to rewrite to a simpler countBy, please feel free 👍

@ockham
Copy link
Contributor

ockham commented Aug 20, 2018

Noting for posterity/future me: As discussed on a video call, I find WpcomChecklist's viewMode prop a bit too magical. My preference would be to instead provide a number of component types to WpcomChecklist, notably a taskType and a headerType (and, if necessary, something like a completedTaskType), but preferrably no checklistType. My hope was that this would give us a somewhat cleaner interface to implement both the regular WP.com checklist, and the banner.

However, I've blocked the JP checklist long enough with my architectural requests, and this PR makes things already a lot better, so I rest my case 😄

ockham
ockham previously approved these changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left a few notes, but overall, LGTM!

@sirreal
Copy link
Member Author

sirreal commented Aug 20, 2018

I'm putting this on hold, it needs a rebase and some rethinking since #26758 landed.

@ockham
Copy link
Contributor

ockham commented Sep 10, 2018

This is what I get from copy-pasta

@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

Thanks @blowery, we tracked it down and fixed in 725dbaf

I'll call this one a tooling and language failure. Thankfully ie 11 is there to save us. This could easily have slipped through the cracks, just by chance the one suite that's run agains ie11 happens to hit this path 😵

@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

Knee jerk reaction– GIVE ME THIS: https://eslint.org/docs/rules/no-implicit-globals

(related context WordPress/gutenberg#9266)

@ockham
Copy link
Contributor

ockham commented Sep 10, 2018 •
edited
Loading

FTR (from a Slack convo with Jon): While there is a no-implicit-globals rule, I'm not positive we should enable it, b/c SSR -- see e.g. WordPress/gutenberg#9266 (node providing some analogs for window globals)

Edit: jinx

@blowery
Copy link
Contributor

blowery commented Sep 10, 2018

@sirreal yeah, this has burned us more than a few times.

@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

Tested instructions from #26758 and it continues to perform as expected. No obvious regressions there.

@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

There's another regression after #26867 that I'm fixing. The banner is only showing when designType === 'blog' which has changed.

@sirreal
Restore updated ChecklistBanner display logic
295894a 
Changed in #26867, regression introduced during rebase.
@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

I've fixed some regressions that had been introduced during rebase which lost changes from #26721 and #26867. It would be good for reviewers to test instructions from those PRs for regressions.

This is working well now in my testing.

@sirreal
Copy link
Member Author

sirreal commented Sep 10, 2018

I restructured in order to be able to remove some eslint namespace ignores.

I've gone through all the commentary and this should be ready for review.

@sirreal sirreal requested a review from a team September 10, 2018 16:39
@sirreal 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] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 10, 2018
@ockham ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 10, 2018
ockham
ockham approved these changes Sep 10, 2018
View reviewed changes
Copy link
Contributor

@ockham ockham left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested:

  • Jetpack site
    • No ChecklistBanner
    • 6 tasks
  • WP.com site without designType
    • No ChecklistBanner
    • 7 tasks
  • WP.com site with designType set to blog
    • ChecklistBanner displayed
    • 9 tasks
    • 'About Page' and 'First Post' links point to correct destinations after completion

:shipit: 🎉 🎈 🎆

@ockham ockham merged commit 29af3b0 into master Sep 10, 2018
@ockham ockham deleted the update/cleanup-checklist-main-show branch September 10, 2018 19:28
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 10, 2018
@sirreal sirreal mentioned this pull request Sep 19, 2018
Merged
sirreal
sirreal commented Sep 19, 2018
View reviewed changes
client/my-sites/checklist/wpcom-checklist/index.js
const location = 'banner' === this.props.viewMode ? 'checklist_banner' : 'checklist_show';

this.props.recordTracksEvent( 'calypso_checklist_task_start', {
checklist_name: 'jetpack',
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a regression, should be new_blog.

Fix in #27304

@jsnajdr jsnajdr mentioned this pull request Nov 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ockham ockham ockham approved these changes

@taggon taggon Awaiting requested review from taggon

Assignees

@sirreal sirreal

Labels
Checklist NUX [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sirreal @matticbot @ockham @blowery