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

feat: add page tabbing #946

Closed
wants to merge 6 commits into from
Closed

Conversation

sbansal3096
Copy link
Contributor

Motivation

Fixes #45

Add page tabbing infrastructure to the Docusaurus site.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

screenshot from 2018-09-05 20-41-38

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 5, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 5, 2018

Deploy preview for docusaurus-preview ready!

Built with commit c163909

https://deploy-preview-946--docusaurus-preview.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

This does not close #45 as user who wanted a tab pagging have to manually include the script inside the docs markdown

 <script>
function openConfig(evt, configOption) {
    var i, tabContent, tabLinks;
    tabContent = document.getElementsByClassName("tabcontent");
    for (i = 0; i < tabContent.length; i++) {
        tabContent[i].style.display = "none";
    }
    tabLinks = document.getElementsByClassName("tablinks");
    for (i = 0; i < tabLinks.length; i++) {
        tabLinks[i].className = tabLinks[i].className.replace(" active", "");
    }
    document.getElementById(configOption).style.display = "block";
    evt.currentTarget.className += " active";
}
document.getElementById('userShowcase').style.display = "block";
document.getElementById("userShowcaseLink").className += " active";
</script>

cc @yangshun @JoelMarcey

Edit: you might want to look at something like #103 (comment)

@sbansal3096
Copy link
Contributor Author

@endiliey
We have 2 options.

  1. Use a plugin like in the link you provided, but it would require HTML and CSS changes to match the formatting of Docusaurus site.
  2. Completely code in HTML, CSS and JS only, but try to make it some general function to make it as easy as possible for other users to use it.

what is your suggestion, how should we proceed?

@endiliey
Copy link
Contributor

endiliey commented Sep 5, 2018

@sbansal3096

I prefer the latter. We should make it easy for user to add tabbing without the need to add script on every doc

@sbansal3096
Copy link
Contributor Author

@endiliey Can you check it once, would something like this work? I added the script function in DocsLayout.js so that it can be used in any doc page.

@endiliey
Copy link
Contributor

endiliey commented Sep 8, 2018

@sbansal3096
It looks much better although I have some concern.

When I go to https://deploy-preview-946--docusaurus-preview.netlify.com/docs/en/next/site-config#mandatory-fields

It doesn't really go to mandatory fields. Try choosing one tab and the right hand TOC doesn't work as expected if we are in certain page tab.

capture

@sbansal3096
Copy link
Contributor Author

@endiliey Yes, you are right.
But when on userShowcase tab, mandatory fields won't work because they are actually not on that page right now. So we would have to link tabbing features with right hand TOC somehow.

@endiliey
Copy link
Contributor

endiliey commented Sep 8, 2018

I guess the requested changes have been communicated clearly.
Goodluck 👍 Ping us again when you've made the changes

@endiliey endiliey changed the title Adding tab paging feature in siteConfig.js feat: add page tabbing Sep 8, 2018
@sbansal3096
Copy link
Contributor Author

@endiliey I have one more concern. Having same options on the tabs and right hand TOC seems to be vague. Both would provide the same functionality of accessing a part of page quickly and easily. What should we do in such a case?
One solution we can have is allowing user to choose between either tabs or TOC.
Or we could format TOC according to the tab selected, with containing options only for the tab selected.

@endiliey
Copy link
Contributor

endiliey commented Sep 8, 2018

we could format TOC according to the tab selected, with containing options only for the tab selected.

I prefer this.

cc @yangshun @JoelMarcey

@sbansal3096
Copy link
Contributor Author

@endiliey Yeah me too, thanks for quick replies. I'll start working on this.

@JoelMarcey
Copy link
Contributor

I prefer that too.

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Please rebase, we have restructured v1 into a separate folder

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

See requested changes below.

In addition, if we go to https://deploy-preview-946--docusaurus-preview.netlify.com/docs/en/next/site-config#mandatory-fields

image

No tabs are active, unlike https://facebook.github.io/react-native/docs/getting-started.html which has gettingStarted as an active tab

I have to click the tab first before it trim all the other tab's content

}
tabLinks = document.getElementsByClassName("tablinks");
for (i = 0; i < tabLinks.length; i++) {
tabLinks[i].className = tabLinks[i].className.replace(" active", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be more specific and use $ regex

replace(/ active$/, "")

This is to avoid replacing a very special edge case nav activeItem active to navItem active in which it should be nav activeItem

for (i = 0; i < tabContent.length; i++) {
tabContent[i].style.display = "none";
}
tabLinks = document.getElementsByClassName("tablinks");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's specificly check if tabLinks exist. There is a chance that tabLinks[num] is undefined and tabLinks[num].className is accessing property of undefined which yields an error. Similar to any below codes

@sbansal3096
Copy link
Contributor Author

@endiliey Hey thanks for reviewing the pull request. https://facebook.github.io/react-native/docs/getting-started.html does not have gettingStarted as starting tab. The tab is default to QuickStart, which in our case won't work as we can have hashtags in the URL.

@endiliey
Copy link
Contributor

Oops, I misconfused the words. You are true, I was referring to Quick start. Nvm, I think this is fine. Just address the requested changes on implementation and it's OK from me.

Let's get review from @yangshun or @JoelMarcey as well

@sbansal3096
Copy link
Contributor Author

@endiliey Thanks a lot. It's really nice of you to reply such quickly. I'll make the changes in implementation soon.

<script
dangerouslySetInnerHTML={{
__html: `
function openConfig(num) {
Copy link
Contributor

@endiliey endiliey Sep 23, 2018

Choose a reason for hiding this comment

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

In addition, I think it should not be named openConfig since its not related to config, its more related to tabbing. Use intention revealing names

The num could be better refactored/changed to the actual id we want to change the class on as well.

I find the name not meaningful enough and using number to refer might not be a good idea.

I recommend something like

function showTabContent(tabContentID) {
  // ....
  var tabContent = document.getElementById(tabContentID):
 //... blablabla
  
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The naming part can be changed, but the num part is a bit tougher. The tab content could have an id which can be passed while calling this function. But the id of right side toc cannot be known beforehand unless user explicitly gives each heading in right side toc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok lets do what we can first.

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

@sbansal3096 Hi 👋 - is it possible to automatically select a tab when you visit a site with tabs? For example you go to https://deploy-preview-946--docusaurus-preview.netlify.com/docs/en/next/site-config and the tabs are shown unselected. And all content is shown. Could you just select "siteConfig Fields" by default so that only that content is shown?

Not a showstopper, but maybe a better user experience.

@sbansal3096
Copy link
Contributor Author

@JoelMarcey Hey I already discussed regarding this with endiliey before, you might wanna look at this, #946 (comment)

@JoelMarcey
Copy link
Contributor

@sbansal3096 Hmmm. Not sure I understand why we couldn't make my suggestion? You can use JavaScript or maybe even just HTML/CSS force a click on one of the tabs in an automated fashion to mimic what a user would do.

Am I missing something?

@sbansal3096
Copy link
Contributor Author

@JoelMarcey What if user adds the hashtag in the URL itself, then the default tab won't work, right? This was the reason I chose to left all tabs deselected initially.

@JoelMarcey
Copy link
Contributor

What if user adds the hashtag in the URL itself, then the default tab won't work, right? This was the reason I chose to left all tabs deselected initially.

Couldn't JavaScript be used to determine which tab should be selected in that case too? I think with some scripting it could be done.

However, I don't feel too strongly about it. If you think the current implementation is the best way to go for now, I am ok moving forward.

@cardinot
Copy link

cardinot commented Oct 4, 2018

This is a great feature; many thanks @sbansal3096 for implementing that. 👏 👏

I noticed the discussion about the unselected tabs at the start... My 2 cents here:

From a user perspective (my view), I'd not (or never) expect to see all the content at the start. Also, it seems that once a tab is selected, you can't go back to the initial state (all unselected) without reloading the page, so, at first glance, this behaviour looks like a bug with the tabs...

I don't know what could be changed to make it work in that way, but I should admit that IMHO:

  • https:/xyz/the-link-to-the-page - should start from the first tab (selected)
  • https:/xyz/the-link-to-the-page#tab2 - if possible, it would be nice to be able to choose a tab from the link
  • https:/xyz/the-link-to-the-page#sect-in-tab2 - if the link points to a section in the tab2, it should start from tab2...

Thanks

@endiliey
Copy link
Contributor

endiliey commented Oct 9, 2018

My 2 cents are the current behavior are indeed weird from user perspective.

@JoelMarcey
Copy link
Contributor

@sbansal3096 @endiliey -- We are going through our pull requests for our next release. We were not sure if this was ready to go yet. I know @yangshun was a bit concerned that this directly modifies DocsLayout, the code of which is on every docs page. Not a showstopper, assuming this is ready, but if it is not ready, then maybe we want to determine whether we can go about this another way.

@JoelMarcey JoelMarcey added the status: needs more information There is not enough information to take action on the issue. label Jan 23, 2019
@sbansal3096
Copy link
Contributor Author

@JoelMarcey I agree, this is surely not ready. It has some issues for concern. I am open for suggestions.

@endiliey
Copy link
Contributor

I'm gonna close this one because I think we already have another better solution shipped in Docusaurus. Thank you for the effort by the way 😉

@endiliey endiliey closed this Mar 21, 2019
@ziscloud
Copy link

@endiliey I read the latest document (2.0.0-alpha.72), but I can not find the 'better solution', where I can get it?

@yangshun
Copy link
Contributor

This issue for Docusaurus v1. If you're using v1, then refer to this page - https://v1.docusaurus.io/docs/en/doc-markdown#language-specific-code-tabs

But we recommend using v2 and it seems like you're using it also. In that case, refer to https://docusaurus.io/docs/markdown-features/tabs

@ziscloud
Copy link

This issue for Docusaurus v1. If you're using v1, then refer to this page - https://v1.docusaurus.io/docs/en/doc-markdown#language-specific-code-tabs

But we recommend using v2 and it seems like you're using it also. In that case, refer to https://docusaurus.io/docs/markdown-features/tabs

@yangshun I know the 'tabs' in the v2, and I also tried it out, but it is not what I want that has been mentioned before

we could format TOC according to the tab selected, with containing options only for the tab selected.

@slorber
Copy link
Collaborator

slorber commented Mar 24, 2021

but it is not what I want that has been mentioned before

@ziscloud It is not clear to me what you want exactly, and why v2 tabs aren't a good fit your you

@kevinmpowell
Copy link

kevinmpowell commented Sep 13, 2021

Not sure exactly what @ziscloud means, but the v2 tabs have fewer features than what's been discussed on this issue. The table of contents does not change when a new tab is clicked, making it very confusing when using tabs and the table of contents features at the same time:
tabs

@slorber what's missing from v2 tabs IMO:

  • Only display headings from the currently selected tab in the table of contents
  • Update the table of contents when a new tab is selected
  • Persist the tab state in the url: mysite.com/docs/button#tab-2
  • Persist table of contents anchor link in the url: mysite.com/docs/button#heading-within-tab-2

@Josh-Cena
Copy link
Collaborator

@kevinmpowell In v2 the Tabs is just a normal React component, and it seems there's no interest in making it any more special than user-defined components (which means it doesn't, and won't, have access to the TOC comp at all, neither would the TOC implementation know about the existence of Tabs): #5343

@yangshun
Copy link
Contributor

My recommendation would be to try not to put headings and stuff into the tabs. It's not a common thing to do IMO.

@adamsoffer
Copy link

adamsoffer commented Aug 26, 2022

Would love to see this feature (paged tabs) get prioritized. It's different than what the current tab component offers. The Stripe.com docs are a great example of this. Notice when you switch tabs between "dashboard" and "api" the urls and table of contents updates.

https://stripe.com/docs/billing/subscription-resource

cc: @endiliey

@slorber
Copy link
Collaborator

slorber commented Aug 26, 2022

@adamsoffer Endiliey is not there anymore unfortunately (https://docusaurus.io/blog/2020/01/07/tribute-to-endi)

That's an interesting feature that I never saw before. That's worth opening a new feature request for that, although I doubt we'd have the bandwidth to implement it, as it doesn't look too easy.

Please post a gif in the feature request, otherwise the Stripe doc may change and we may lose context

CleanShot 2022-08-26 at 17 36 58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA status: needs more information There is not enough information to take action on the issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add page tabbing infrastructure