-
Notifications
You must be signed in to change notification settings - Fork 817
My Jetpack: Onboarding i3 - Init tab panel #43595
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
My Jetpack: Onboarding i3 - Init tab panel #43595
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
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.
Works good, thanks for the quick implementation!
I'm leaving some of the feedback below; most urgent functional feedback is that we should probably explicitly mention which paths are supported by tab system - or revert to the default path if some custom not existing one is provided.
@@ -4,7 +4,7 @@ export const MY_JETPACK_MY_PLANS_PURCHASE_NO_SITE_SOURCE = 'my-jetpack-my-plans- | |||
export const MY_JETPACK_PRODUCT_CHECKOUT = 'my-jetpack-product-checkout'; | |||
|
|||
export const MyJetpackRoutes = { | |||
Home: '/', | |||
Home: '/:tab', |
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.
Should we be more exhaustive here which options for tabs are available? As I understand, currently it might match anything other than defined paths?
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.
React Router does not support pattern matching here, but we can validate the value when using it.
[ navigate, params.tab ] | ||
); | ||
|
||
const tabRenderer = useCallback( ( tab: { name: string; title: string } ) => { |
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.
We can benefit from more explicit typing here.
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.
Do you have any suggestions about it?
const navigate = useNavigate(); | ||
|
||
const onTabSelect = useCallback( | ||
( tabName: string ) => { |
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.
We can benefit from more explicit typing here.
return ( | ||
<TabPanel | ||
className={ styles[ 'tab-panel' ] } | ||
initialTabName={ params.tab || 'overview' } |
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.
Should we perhaps also replace the path to /overview
if the initial tab name is not provided?
Also, I would probably go for some globally defined constant "DEFAULT_INITIAL_TAB" instead of hardcoding it here in code.
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.
Yes, it defaults to /overview
if nothing is provided or if an invalid tab name is supplied.
tabs={ [ | ||
{ | ||
name: 'overview', | ||
title: __( 'Overview', 'jetpack-my-jetpack' ), | ||
}, | ||
{ | ||
name: 'products', | ||
title: __( 'Products', 'jetpack-my-jetpack' ), | ||
}, | ||
{ | ||
name: 'help', | ||
title: __( 'Help', 'jetpack-my-jetpack' ), | ||
}, | ||
] } |
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.
Maybe it's better to define it as a variable outside of the component?
I also lack (type) connection between this array and tabComponentMap
. It could be more exhaustive that options provided here should match the mapping and vice-versa.
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.
Maybe it's better to define it as a variable outside of the component?
That doesn't play nice with translations as the components update but the variable outside the component never updates/re-renders.
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 have extracted it to a function instead of a variable that works well with translations.
20aba9b
to
557e310
Compare
…nboarding-i3-basic-tab-panel
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 believe it's almost there but still would recommend the routing to be more in sync with UI.
if ( tabName !== params.section ) { | ||
navigate( `/${ tabName }` ); | ||
} |
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.
should we have a guard here, to prevent setting random tab names (and blank page)?
or better, use replaceState() to keep the url in sync? otherwise /blabla
will display "Overview" page, while not critical - it's not perfect as well.
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.
But since we control the tabs, where will the random value come from?
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.
right, that's true! thanks, I think then the only case is if I input manually #/foobar
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.
the only case is if I input manually
#/foobar
That won't do anything because no route is registered corresponding that path
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 have fixed it in ae26423
You can see what happens if I update the URL manually. The key
prop will reset the state when the URL is updated manually.
Screen.Recording.2025-05-26.at.7.02.34.PM.mov
const initialTab = isValidMyJetpackSection( params.section ) | ||
? params.section | ||
: MY_JETPACK_SECTION_OVERVIEW; |
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 believe we could have this value behind useState( () => return initialTab and maybe do replaceState() )
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.
Why? The URL should reflect immediately because TabPanel
calls that onSelect
with the initialTabName
on mount and it in turn updates the URL accordingly. We don't need to manage any state here.
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.
alright, thanks for making it clear, what do you think about the replaceState()
in case isValidMyJetpackSection() === false
?
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.
It already happens, the URL is reflected immediately on mount. We don't need to perform any magic here.
Screen.Recording.2025-05-26.at.6.58.05.PM.mov
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.
Thanks, that is ready to be a base for all other PRs!
49b56b1
into
feature/my-jetpack/onboarding-i3
Completes MYJP-129
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Screen.Recording.2025-05-23.at.5.22.06.PM.mov