Skip to content
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

use wc-admin urls in devdocs #2827

Merged
merged 2 commits into from Aug 22, 2019

Conversation

@rrennick
Copy link
Collaborator

commented Aug 21, 2019

Fixes #2813

This PR changes the URLs used in the DevDocs page to use wc-admin URLs instead of fully qualified dashboard URLs.

Detailed test instructions:

  • Ensure navigation among regular dashboard pages, WC Admin, DevDocs main & component pages works without issue.

Changelog Note:

Fix: Bug navigating from DevDoc component pages to WP dashboard pages.

@rrennick rrennick requested a review from woocommerce/wc-admin Aug 21, 2019

@rrennick rrennick added this to In Progress PRs (for automation purposes) in wc-admin via automation Aug 21, 2019

@@ -63,15 +62,13 @@ export default class extends Component {
component ? (
componentName
) : (
<Link href={ getAdminLink( `?page=wc-admin&path=/devdocs/${ filePath }` ) }>
<Link href={ `admin.php?page=wc-admin&path=/devdocs/${ filePath }` }>

This comment has been minimized.

Copy link
@jeffstieler

jeffstieler Aug 22, 2019

Contributor

What changed in getAdminLink() to make it unsuitable for this usage?

This comment has been minimized.

Copy link
@rrennick

rrennick Aug 22, 2019

Author Collaborator

In the <Link /> component an onClick handler is added if the link type is wc-admin. When the link is clicked it appends the link href to the end of the base wc-admin URL. By using the getAdminLink() function the onClick is using full URI.

The alternative change that would work is changing the link type in the getAdminLink() call.

This comment has been minimized.

Copy link
@jeffstieler

jeffstieler Aug 22, 2019

Contributor

Oh, it looks like the <Link> component internally calls getAdminLink(). 👍

@jeffstieler
Copy link
Contributor

left a comment

Looks good. Optionally explicitly pass the type="wc-admin" to the <Link> component, but this can be 🚢ed as is.

@rrennick

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2019

Optionally explicitly pass the type="wc-admin" to the component

@jeffstieler thanks for the suggestion. I added these in 0065615 .

@rrennick rrennick merged commit ed07b2b into master Aug 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

wc-admin automation moved this from In Progress PRs (for automation purposes) to Done Sprint 23 (August 13 - August 26) Aug 22, 2019

@rrennick rrennick deleted the fix/2813 branch Aug 22, 2019

@psealock psealock added [estimate] 1 and removed [estimate] 1 labels Aug 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.