-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
Avoid a potential flash of the placeholder without showing the TaskList.
const isTaskListEnabled = | ||
isOnboardingEnabled() && | ||
! taskListHidden && | ||
! window.wcAdminFeatures.homepage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffstieler if #4418 is merged first this should be updated to ! getFeatureFlag( 'homepage' );
Opened a PR on this here. A couple of small things I didn't get to:
So cool to see this coming together - it's looking really good! :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to reverting that change for sure.
The task list looks weird just appearing. It looks like it depends on two options woocommerce_task_list_complete
and woocommerce_task_list_hidden
, but only the later is preloaded. Should we preload them both?
@@ -311,7 +314,7 @@ class CustomizableDashboard extends Component { | |||
<Fragment> | |||
{ isTaskListEnabled && ( | |||
<Suspense fallback={ <Spinner /> }> | |||
<TaskList query={ query } inline={ isDashboardShown } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated nit: Maybe rename isDashboardShown
to isTaskListEnabled
so it matches the use Home page
|
||
/** | ||
* WooCommerce dependencies | ||
*/ | ||
import { Card, List, MenuItem, EllipsisMenu } from '@woocommerce/components'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
client/task-list/index.js
Outdated
label={ __( 'Task List Options', 'woocommerce-admin' ) } | ||
renderContent={ () => ( | ||
<div className="woocommerce-task-card__section-controls"> | ||
<Button onClick={ () => this.hideTaskCard( 'remove_card' ) }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another unrelated comment: It always bugged me that clicking this didn't cause the task list to disappear immediately or at least show a spinner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to handle this in a follow up?
'Purchase, install, and manage your extensions directly from your dashboard', | ||
'wooocommerce-admin' | ||
), | ||
icon: 'extension', | ||
container: null, | ||
onClick: () => | ||
remainingProductIds.length ? toggleCartModal() : null, | ||
visible: productIds.length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking "Purchase & install extensions" doesn't do anything for me. Maybe this should be visible: remainingProductIds.length
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's functioning correctly (this is unchanged in the PR) - is it showing complete? If you selected a plugin and have it installed, the step will display as completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe it because I'm doing odd things like const isTaskListEnabled = true;
hard coded to get it to come up.
window.location = getAdminLink( | ||
'admin.php?page=wc-admin&reset_profiler=1' | ||
); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps good for a follow up: We should use <Link />
component with these list items. This onClick
causes a page refresh where one isn't required. Clicking "Store details" should not refresh the page. Secondarily, it removes the "preview" address in the lower right hand side which is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd definitely like to tackle this in a follow up - at a guess, this is likely to be more involved than changing to a link component since we're potentially going to need to reset the "completed" flag for the task list (and think through the impact to tracking of that completion).
* Colors * Only show time estimation for incomplete tasks * Don't show chevron on completed tasks. Co-authored-by: Jeff Stieler <jeff.m.stieler@gmail.com>
Reworked in 98b8aa9. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked in 98b8aa9.
It still makes a request, even though both options are preloaded. In https://a8c.slack.com/archives/C9K5T0XSP/p1590452504044500?thread_ts=1590435699.034400&cid=C9K5T0XSP we discovered that hydration isn't working as expected with lazy loaded components. Maybe thats happening here.
Everything is looking good to me, just a prop type violation and the rest to be addressed in follow ups.
/** | ||
* If the task list has been completed. | ||
*/ | ||
taskListComplete: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres a proptype violation here, "Invalid prop taskListComplete
of type string
supplied to Layout
, expected boolean
." I found it when the task list wasn't visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to reproduce this - can you provide steps?
'Purchase, install, and manage your extensions directly from your dashboard', | ||
'wooocommerce-admin' | ||
), | ||
icon: 'extension', | ||
container: null, | ||
onClick: () => | ||
remainingProductIds.length ? toggleCartModal() : null, | ||
visible: productIds.length, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe it because I'm doing odd things like const isTaskListEnabled = true;
hard coded to get it to come up.
Fixes #4096.
This PR seeks to update the appearance and functionality of the
<TaskList />
component by:Another change made in this PR is to not use the
<TaskListPlaceholder />
when the options are being requested as it causes the placeholder to show in cases where the task list isn't displayed. This is an improvement for those cases, but perhaps makes the scenarios where the task list is displayed more jarring, as it "just appears" on fast connections (the placeholder is just forSuspense
now).I'm open to reverting that change for sure.
Accessibility
prefers-reduced-motion
Screenshots
@jameskoster does this look OK? ☝️
Detailed test instructions:
Changelog Note:
Tweak: update design of store setup task list.