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

Dynamically rendering PageNavigators. #297

Open
wants to merge 3 commits into
base: versions/3.0
Choose a base branch
from

Conversation

nothuman2718
Copy link
Contributor

@nothuman2718 nothuman2718 commented Mar 31, 2024

Dynamically rendering PageNavigators.
With this change, the Next Prev page navigators will be generated dynamically.

Review time on a scale of 1-5 is 3.

@garg3133
Copy link
Member

garg3133 commented Apr 1, 2024

This is cool. Thanks a lot, will review it soon.

@nothuman2718 nothuman2718 force-pushed the pagenavigators branch 2 times, most recently from fa0ab37 to 2406d5f Compare April 1, 2024 13:55
@nothuman2718
Copy link
Contributor Author

nothuman2718 commented Apr 1, 2024

Requested changes are pushed. Please review it when you had time.

.gitignore Show resolved Hide resolved
@nothuman2718
Copy link
Contributor Author

@beatfactor Should I update .gitignore then?

@beatfactor
Copy link
Member

@beatfactor Should I update .gitignore then?

it's fine, you can leave it.

@garg3133
Copy link
Member

garg3133 commented Apr 5, 2024

@nothuman2718 Try not to force push on the PR every time you make some changes, it makes it hard to assess the improvements you've made after the last review.

@nothuman2718
Copy link
Contributor Author

Sure. Noted!!
Will not repeat again.

@garg3133
Copy link
Member

garg3133 commented Apr 5, 2024

For a few pages, the titles that we have in the page navigators are different from the titles we have on the sidebar (on the main website) while they're going to match exactly with this PR.

image

I think it might be a good idea to keep these titles for page navigators and not match all the page navigator titles with the titles on the sidebar. This would be especially useful when we have situations like below where the "Next Page" such says "Introduction" while it should be something like "Introduction to Mobile Testing".

image

This could be easily achieved by supplying an alternative title for each page (if applicable) in the navItems list.

@beatfactor What do you think?

@beatfactor
Copy link
Member

Yes, we should have an alternative link title in the navItems list.

@nothuman2718
Copy link
Contributor Author

think it might be a good idea to keep these titles for page navigators and not match all the page navigator titles with the titles on the sidebar. This would be especially useful when we have situations like below where the "Next Page" such says "Introduction" while it should be something like "Introduction to Mobile Testing".

Sure.

@nothuman2718
Copy link
Contributor Author

@garg3133 Please have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants