Add existing TaskList component to new home screen #4378
Conversation
| if ( isOnboardingEnabled() ) { | ||
| const options = getOptions( [ | ||
| 'woocommerce_task_list_hidden', | ||
| ] ); |
There was a problem hiding this comment.
Should we pre-load this option? It seems like holding off on render while waiting for this request is unnecessary. What do you think?
There was a problem hiding this comment.
We can preload this option but that doesn't remove the need for the placeholder - it's in use as the Suspense fallback as well.
There was a problem hiding this comment.
Sounds good, 👍 to preloading
There was a problem hiding this comment.
It looks like this is already preloaded:
woocommerce-admin/src/Features/Onboarding.php
Line 504 in 5f3223f
There was a problem hiding this comment.
Is that file getting loaded on the homescreen route? I think we do conditional loading of OBW logic. I haven't fired this one up, but I remember it waiting on this option to be fetched.
There was a problem hiding this comment.
Nevermind, that option is available at wcSettings.preloadOptions.woocommerce_task_list_hidden.
I'll take a closer look today.
There was a problem hiding this comment.
Its some kind of wc-api thing because if I hard code the values, then it renders as soon as the chunk has arrived. This looks fine to me, #4144 will ensure the preloaded option is available immediately.
|
|
||
| const TaskListPlaceholder = ( props ) => { | ||
| const { | ||
| numTasks = 5, |
There was a problem hiding this comment.
Nitpick: May as well destructure in the function's argument list
| numTasks = 5, | ||
| } = props; | ||
| return ( | ||
| <div className="woocommerce-task-dashboard__container"> |
There was a problem hiding this comment.
How about this class name?
| <div className="woocommerce-task-dashboard__container"> | |
| <div className="woocommerce-task-list__container"> |
There was a problem hiding this comment.
That's the existing class name here: https://github.com/woocommerce/woocommerce-admin/blob/master/client/dashboard/task-list/index.js#L373
| export default compose( | ||
| withSelect( ( select ) => { | ||
| const { | ||
| getOptions, |
There was a problem hiding this comment.
just an FYI that this is getting refactored in #4144. If this is merged first, I'll update the code.
42d813e to
d7246f9
Compare
There was a problem hiding this comment.
Looking great @jeffstieler ! Working well and code looking good.
8523236 to
038cef8
Compare
Progress towards #4096
This PR seeks to render the existing
<TaskList />component on the new home screen (before modifying it to suit the new designs).A new
TaskListPlaceholdercomponent has been added to use during lazy load and API fetch.It also modifies the test configuration to allow for dynamic
import()s.Accessibility
prefers-reduced-motionScreenshots
Detailed test instructions:
Changelog Note: