-
Notifications
You must be signed in to change notification settings - Fork 278
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.
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
UX/UI Improvement for Pie Chart Feature on the New All Traffic Module #2644
Comments
IB ✅ |
The fix for this may also address and close out #2660 |
QA ❌This passed using Chrome but Failed using Safari Steps:
Sending back to CR to adjust for Safari @tofumatt |
Hm... i can't reproduce it on my end. @tofumatt do you have Safari? I am on Ubuntu and don't have it. Could you please try it on your end? |
@cole10up Can you go to the same site and click in the exact order as you did in the GIF with Chrome? I couldn't reproduce that issue in Safari, but from how it's coded and the data I'm seeing in your GIF that shouldn't be a browser-specific issue unless the order of the chart data isn't consistent. Can you check to see if that exact approach happens in Chrome too? The chart selection is pretty spotty in general and something we need to fix (see #2714), so I'm wondering if that's what's happening here… |
@cole10up @tofumatt I think overall this UX improvement is not super critical for the functionality of the widget, and we already have #2714 in place to fix these inconsistent failures. Unless there is a broader issue where e.g. we'd select the wrong slice or display incorrect data, I think we can wave this through for the release. We should be strict when testing #2714 - the only reason we're not doing that one now is that it will involve a lot more time to explore than we have right now. |
So, after testing this out locally with the data set provided by @cole10up, turns out this isn't actually a bug with the implementation of this feature, but of how the colours are displayed/chosen by the charts library/component. It caused us to think the selection was being changed in different date ranges because the colour changed. But take a look at the value selected in this GIF: The value stays the same and the selection is correct, but the colour-changing makes for a very confusing UX. We should maybe make the values consistent between selections, but that's outside the scope of this issue. So I think the QA here is actually good 😆 |
(@cole10up confirmed QA was otherwise good, so we're set! 👍🏻 ) |
Bug Description
This is a UI improvement suggestion. For the New All Traffic module on Site Kit Dashboard. On the pie chart, select a slice, and then change the date selection to another period, i.e. 90 days. The module reloads and the correct number of users is displayed for that slice, but it is no longer selected. It would be good to see the slice continued to be highlighted so the user knows the user stats are related to that slice on first look.
Steps to reproduce
90 days
Screenshots
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
site-kit-wp/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetV2/UserDimensionsPieChart.js
Line 80 in 562423f
FORM_ALL_TRAFFIC_WIDGET
as a value (eg.selectedRow
or something similar). Equally when the selection is cleared, set theselectedRow
toundefined
)UserDimensionsPieChart
usingconst selectedRow = useSelect( ( select ) => select( CORE_FORMS ).getValue( FORM_ALL_TRAFFIC_WIDGET, 'selectedRow' ) );
selectedRow
is notundefined
in the chart'sonReady
callback (site-kit-wp/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetV2/UserDimensionsPieChart.js
Lines 74 to 79 in 6e2aa51
Ensure that you check for
undefined
, not just truthiness, as the selected row index could be0
.Test Coverage
Visual Regression Changes
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: