google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 289 forks source link

Explore More Elegant Solution For Re-Focusing The First Input In Audience Selection Panel #8968

Open zutigrm opened 3 months ago

zutigrm commented 3 months ago

Feature Description

Currently when audience sync action is invoked on first panel opening, first input in the panel is not focused afterwards. There is also a preview block, shown before the inputs are loaded, although in my tests focus is lost after re-sync action happens.

Current implementation is in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js after TODO comment

Screenshot 2024-09-17 at 14 08 30

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

benbowler commented 1 month ago

@tofumatt @zutigrm I think the screenshot and AC are incorrect here, the ticket is about the AudienceItems list, first checkbox not focussing. It's not about the "Got It" button on the SetupCTA.

https://github.com/google/site-kit-wp/blob/8d1a73f3d53612b35406a1d1177a20ed53683eab/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js#L70-L81

Screenshot 2024-09-17 at 14 08 30

Can you update the AC? Then I will look at the IB.

benbowler commented 1 month ago

To test, open "Change Groups" panel and run googlesitekit.data.dispatch( 'modules/analytics-4' ).syncAvailableAudiences(); in the developer console, you will see the first checkbox looses focus.

tofumatt commented 1 month ago

@benbowler Thanks! I've amended the ACs, if they look good this is ready for an IB, but if not feel free to edit them and assign me to review the ACs, happy to do that if I'm still missing it 😄

zutigrm commented 1 month ago

@benbowler Yes indeed, not sure how I managed to include the wrong screenshot, but yes issue is about the focus of the first item

benbowler commented 1 month ago

@zutigrm testing this the activeElement is returned to the first checkbox after syncing in my testing, using this technique.

Have I missed something/is there a way to repeat the issue you were seeing when you wrote this ticket?

zutigrm commented 1 month ago

@benbowler The issue was that first input did not receive focus after selection panel is first opened. This issue is not about fix, since I already implemented the one mentioned in the issue. This one is about exploring about more elegant solution, since I did a quick fix, and didn't have enough time to explore further. If you can find more elegant solution, it can be used instead of the current one I implemented, otherwise, we don't need to do anything about it and we can close the issue

benbowler commented 1 month ago

Thanks @zutigrm an option would be to wrap the panel in a FocusTrap, which is how material Dialog components work.

I'll draft and IB on this basis next week.

ivonac4 commented 1 month ago

Based on the conversation on Slack here, we agreed this is a P2 ticket and I moved it to the backlog. It is only about exploring for more elegant solution for the quick fix Aleksej implemented. We will not invest time in this ticket anymore.

Thanks all!

@zutigrm @benbowler @techanvil