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

docs(api.pug): change to redirect to mongoose API page #12223

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

hasezoey
Copy link
Collaborator

@hasezoey hasezoey commented Aug 6, 2022

Summary

This PR addresses #12217, with the approach of using a script to redirect based on the anchor

fixes #12217

Note: This description was changed later, so the earlier comments might reference older information

@hasezoey
Copy link
Collaborator Author

hasezoey commented Aug 9, 2022

Merged latest master to resolve conflicts

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about this breaking existing links to the api page. Any anchor that links to a header that isn't in /api/mongoose will break.

We'd like to make this change eventually, that's why we added api_split.pug, but still wary of broken links. What do you think @hasezoey ?

@hasezoey
Copy link
Collaborator Author

hasezoey commented Aug 9, 2022

I'm a little concerned about this breaking existing links to the api page. Any anchor that links to a header that isn't in /api/mongoose will break.

like i said (in the PPS), with this current way, it would break all anchor links to the old page

the other workaround would be to redirect via script, with anchors (which i did not do originally, because it potentially loads longer, or not at all when JS is disabled)

@hasezoey
Copy link
Collaborator Author

@vkarpov15 should i change it to a js script that redirects to the correct page with anchor instead of a meta-refresh?

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 7, 2022

i just noticed that because api.html includes all pages, just redirecting to mongoose.html (even with proper anchor) would not be correct or working, so what should we do about that?
options would be:

  • dont redirect with anchor at all
  • only redirect to mongoose.html and have only those anchors that exist there working
  • implement a custom script to redirect properly everywhere

@vkarpov15

@hasezoey hasezoey added the docs This issue is due to a mistake or omission in the mongoosejs.com documentation label Sep 7, 2022
@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 9, 2022

  • Updated Implementation to be a script that tries to use the anchor to select the file (base case is api/mongoose.html)
  • change all links that used api.html to link to the proper place
  • add proper title to all generated API outputs (previously pug-variable title was unset)
  • change api.js to only export the docs array

The only thing that still needs to be updated is search.js, but i dont know enough about it so i would like to ask @vkarpov15 to take care of that

@hasezoey hasezoey added this to the 6.7 milestone Oct 6, 2022
@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 6, 2022

added to the 6.7 milestone to be considered for merge, because i think this PR is ready and handles all (that i know of) cases

@vkarpov15
Copy link
Collaborator

We'll hold off on this one until 7.0, I'm still a little uncomfortable with merging this one due to SEO concerns. But should be fine to merge with 7.0 release.

@vkarpov15 vkarpov15 modified the milestones: 6.7, 7.0 Oct 24, 2022
@hasezoey
Copy link
Collaborator Author

We'll hold off on this one until 7.0, I'm still a little uncomfortable with merging this one due to SEO concerns. But should be fine to merge with 7.0 release.

sure, would it be alright if some changes (like links to api.html in code & guides) are cherry-picked and put in another PR for direct merging?

@vkarpov15
Copy link
Collaborator

@hasezoey sure

@hasezoey
Copy link
Collaborator Author

will hold off on updating the merge conflicts until other URL changes have been merged (like #12644 and a upcoming PR for #12645)

@hasezoey
Copy link
Collaborator Author

hasezoey commented Dec 2, 2022

Rebased on latest 6.8 branch, fixes because of #12644 and #12645

PS: because this was rebased on 6.8 instead of the base (which is master), there are a lot of unrelated commits showing, they should be gone again once changing the base to 7.0 and having 6.8 merged in to 7.0

@hasezoey
Copy link
Collaborator Author

hasezoey commented Jan 7, 2023

force-push to rebase on latest master (clean-up the PR commits from the 6.8 branch)

also changed the base to 7.0 as per the milestone and discussion previously

@hasezoey hasezoey changed the base branch from master to 7.0 January 7, 2023 14:06
@hasezoey
Copy link
Collaborator Author

rebased onto latest 7.0 to lessen commit history (making it easier to see changes), also to check if everything works

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@vkarpov15 vkarpov15 merged commit 94befc2 into Automattic:7.0 Feb 13, 2023
@hasezoey hasezoey deleted the changeAPItoRedirect branch February 13, 2023 18:42
@hasezoey
Copy link
Collaborator Author

hasezoey commented Feb 13, 2023

@vkarpov15

just asking to not forget it:
could you update the search to use the actual pages, i did not update it because i am not quite sure how it works (the last time i tested and the current live version still only links to api.html)
(maybe i will try to figure it out and make a PR at some point)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] API documentation is generated multiple times
2 participants