-
Notifications
You must be signed in to change notification settings - Fork 123
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
Show all tabs when printing #1498
Conversation
Hi @jonahtanjz thanks for your work on this PR!
Actually I don't have a strong opinion of whether it should be printed, because I'm not sure why a tab would be disabled in the first place (can't really think of a use-case). So I think for brevity sake, I'm alright with them being printed. Since if the user wants to show the tab to the viewers in the first place (although disabled), the user probably wants the viewer to be aware of the tab's existence so I think it makes sense for the disabled tab to appear in the print.
I think it looks decent! I think the font bolding for each tab's header and the underlining of the tab group header makes sense. I didn't have any trouble discerning between tab groups and tabs, and their respective content. Seems quite clear to me. Overall everything looks good to me, just have some suggestions on how to make things a little more succinct using bootstrap's display utilities. The implementation seems to be working well. Good job! :-) |
@jonahtanjz can provide some screenshots for the feature in action? |
@damithc posting on behalf of @jonahtanjz since I still have the page open. |
Thanks for the screenshots @wxwxwxwx9 |
Thanks @wxwxwxwx9 for the review and posting the screenshots 👍 Yup, agree that using bootstrap's display utilities will make things more succinct. Will make the changes accordingly :)
I think it's possible to do it for normal tabs but I'm not very sure if this format will work well with tab groups. Maybe for tab groups we can keep the current formatting with the box but each of the tabs will be displayed as a single tab inside the box. What do you think about this @damithc? |
Need to see how it looks. The goal is to visually indicate to the reader that the content came from separate tabs, as there was a reason for them to be in tabs in the first place (most likely because they were alternatives) and that additional information should be preserved if possible. |
I've come up with something like this. Using the same example from the screenshot provided above, each of the |
Looks good @jonahtanjz I don't think the tab group font should be bigger than the top level tab names, to preserve the hierarchy. |
Alright noted. I have changed the font size to match with the rest of the headers :) |
:class="{hide:!show}" | ||
> | ||
<div class="nav-tabs printable-tab-header d-none d-print-flex"> |
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!
Just one question: could we merge this with the existing header
below? similar to what you did for TabGroup
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.
Yup it's possible. Thanks for the suggestion and I've made the changes accordingly :)
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.
Lgtm 👍
What is the purpose of this pull request?
Overview of changes:
Resolves #1491.
tab-group
will be grouped together.Anything you'd like to highlight / discuss:
tab-group
fine? Do let me know if this can be improved on.Testing instructions:
tabs
andtab-group
Proposed commit message: (wrap lines at 72 characters)
Support printing of all tabs
Tabs are supposed to contain alternative contents.
Readers can click on each tab to see its content. However,
when a page is printed or converted to PDF, the reader can
only see the content of the active tab.
Let's display all the tabs and its content when it is printed.
Checklist: ☑️