Add and use Checklist components, remove blocks #26618
Conversation
sirreal
force-pushed
the
add/checklist-components
branch
3 times, most recently
from
8016cf9 to
67280c4
Compare
sirreal
added
[Status] Needs Review
|
Interesting impact on build. I'm not sure what's happened, it seems that a shared chunk has now but put into two other chunks. cc: @jsnajdr http://iscalypsofastyet.com/branch?branch=add/checklist-components Full output |
sirreal
added
[Status] Needs Review
| <div | ||
| className={ classNames( 'checklist', { | ||
| 'is-expanded': ! this.state.hideCompleted, | ||
| 'hide-completed': this.state.hideCompleted, |
There was a problem hiding this comment.
Hmm, reading these CSS classes like this, they seem kinda redundant...?
There was a problem hiding this comment.
I agree, however that's not been introduced here. I took a quick look and cleanup is more involved than I'd like to include in this PR. Let's leave it for now.
| &.is-completed { | ||
| order: 0; | ||
| } | ||
|
|
client/components/checklist/task.js
Outdated
| onClick={ onDismiss } | ||
| aria-pressed={ | ||
| /* eslint-disable-next-line no-nested-ternary */ | ||
| onDismiss ? ( completed ? 'true' : 'false' ) : undefined |
There was a problem hiding this comment.
See https://github.com/Automattic/wp-calypso/pull/26216/files#r208360821 😬
One thing you could consider is inlining the entire
<ToggleContainer />, which might make things actually more readable:{ onDismiss ? <Focusable onClick={ onDismiss } aria-pressed={ !! completed } // Do we really need the bool strings ('true'/'false') here? /> : <div /> }
There was a problem hiding this comment.
Applied your suggestion in 540e0bd. I'm a bit indifferent as to the clearer approach so we can leave whatever you think is best.
Do we really need the bool strings ('true'/'false') here?
I believe this is the correct way to do it. It is how official docs show it: https://reactjs.org/docs/accessibility.html#wai-aria
Note that these are "true"/"false" strings in the DOM, a bit different from some other boolean-like DOM attributes, e.g. disabled.
| const completed = reduce( | ||
| tasks, | ||
| ( count, { id, completed: taskComplete } ) => | ||
| taskComplete || get( taskStatuses, [ id, 'completed' ] ) ? count + 1 : count, |
There was a problem hiding this comment.
Worth turning
( { id, completed } ) => ! completed && ! get( this.props.taskStatuses, [ id, 'completed' ] )into a method since we're using it in multiple places?
There was a problem hiding this comment.
This only exists in one place in the banner now, so we can leave it. The banner, ideally, will be implemented in another way eventually.
The other usage was removed by 479f892, after I realized since we're not using connected Tasks anymore, we can always infer checklist completion based on children as they should have a completed prop.
There was a problem hiding this comment.
The other usage was removed by 479f892, after I realized since we're not using connected
Tasksanymore, we can always infer checklist completion based on children as they should have acompletedprop.
This is very awesome!
|
|
||
| `Checklist` and `Task` are components used to render checklists. | ||
|
|
||
| If your checklist tasks use checklist state from Redux, you can use `Task` from `blocks/checklist/tasks` to pull state by the `id` prop. |
There was a problem hiding this comment.
No more Task block, is there?
| ### `inferCompletedCount { bool } - default: false` | ||
|
|
||
| If `Task` children will have a provided `complete` prop, the `Checklist` will count the completed | ||
| tasks. This will not work when using connected tasks (see `blocks/checklist/task`). |
|
'Do It' buttons still don't work:
|
sirreal
force-pushed
the
add/checklist-components
branch
2 times, most recently
from
36b772d to
ac2957d
Compare
|
|
||
| renderPlaceholder() { | ||
| return ( | ||
| <div className={ classNames( 'checklist', 'is-expanded', 'is-placeholder' ) }> |
There was a problem hiding this comment.
No need for classNames here to generate static class. However, the visibility toggle doesn't work on the placeholder, but it could. Let's leave this as it will be useful then, and handle that in a follow-up PR.
|
Sorry for the premature review request @ockham. I've ironed out the kinks and applied suggestions. This is ready for another look, thanks! 🙇 |
sirreal
removed
the
[Status] Needs Review
sirreal
force-pushed
the
add/checklist-components
branch
from
d1ee316 to
2244a56
Compare
|
Rebased |
sirreal
added
[Status] Needs e2e Testing
[Status] Ready to Merge
and removed
[Status] Needs Review
| siteId, | ||
| siteSlug, | ||
| tasks: tasksFromServer ? mergeObjectIntoArrayById( tasks, tasksFromServer ) : null, |
There was a problem hiding this comment.
Sorry for this but we can't just delete the line of code since the response from the server-side can contain extra information besides whether a task completes or not. Let me give you an example. The URL of the very first post can be changed based on themes. So the server-side finds and returns its URL then this mergeObjectIntoArrayById applies the new URL to the corresponding task. Here's a link to the server-side code: 1efb3-pb
You don't need to do anything. I will handle this problem in my PR: #26721
There was a problem hiding this comment.
Ugh, I missed that in my review. Sry.
Adds Components from #26216
Uses components for WooCommerce Store checklist
Uses components for ChecklistShow
Removes blocks (discourage block usage)
The goal here is to keep the clear improvements from #26216 outlined in #26015:
connectAnd drop the bits that may not move clearly in the direction we'd like to take:
TaskblockThis should be a good first step in the incremental improvements around checklists. After this, the Woo checklist should not need work, and we can focus on work around the site onboarding checklists.
Implementing ideas from #26015
Closes #26030 (supersedes)
Closes #26216 (supersedes)
Closes #25958
Testing
No functional or visual changes. Testing instructions adapted from #26216 (will add pings if this is considered a good direction).
Checklists may be used in 4 places. The functionality should remain unchanged for all of them:
Functionality:
Checklists in use to test:
/stats/day/SITE) (@taggon / @fditrapani)/plans/my-plan/SITE) (@Automattic/luna)/store/SITE) (@timmyc)Drop the following in https://calypso.live/devdocs/playground?branch=add/checklist-components