-
Notifications
You must be signed in to change notification settings - Fork 140
Support opt-out of printing certain tabs #1612
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
Support opt-out of printing certain tabs #1612
Conversation
raysonkoh
left a comment
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! Have tested with some small examples and verified that it is working as intended.
Thanks @jonahtanjz!
|
@MarkBind/active-devs let's do a release after this PR is merged. |
|
There's some changes to the implementation as discussed here #1605 (comment) @raysonkoh, this PR is not yet updated. Prof @damithc, I will most likely make the changes and update the PR by tomorrow night! |
Got it. Thanks @jonahtanjz |
|
@ang-zeyu Noticed that we can't really remove the current usages of |
a0baf0d to
22089d0
Compare
|
@ang-zeyu Thanks for the suggestion 👍 Have changed |
| text-decoration: underline; | ||
| } | ||
| .printable-tab-group-header.tab-group-print-block { |
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.
@ang-zeyu Thanks for the suggestion 👍
Have changed
v-showandd-print-block/flexto our own custom classes. Needed to add them individually totab-groupandtabso that we can increase the specificity from there.no-printis added to the global css file with!importantso that it can be reused by other components as well once we have changed out all the other usages of!important.
hmm... not sure I follow, what's the addition tab-group-print-block for? (the variants in tab.vue as well)
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.
Ahh seems like css that appear the latest will take effect anyway if same level of specificity. Initially added the tab-group-print-block so that it can help with increasing the specificity over the dispaly: none rules to ensure that it is always overriding it.
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.
Ahh seems like css that appear the latest will take effect anyway if same level of specificity.
yes, one of the main reasons its named cascading style sheets. Let's remove it then!
packages/vue-components/src/Tab.vue
Outdated
| margin: 0; | ||
| } | ||
| .tab-pane.hide { |
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.
would tab-pane-hide work instead? To avoid adding empty classes below to increase the specificity
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.
Seems like there is a css rule in bootstrap, .tab-content > .active, that is more specific than .tab-pane-hide. We can also override that by using .tab-content > .tab-pane-hide in our scoped css but that would mean having to add .tab-content > .printable-tab-pane in the print rule as well. Should we go with this as this doesn't seem to resolve the empty classes issue?
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.
How about using <style scoped>? It adds data-****** attributes under the hood and may be sufficient to override .tab-content > .active's specificity.
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 this works 👍 I've updated the PR accordingly.
ang-zeyu
left a comment
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 👍. Thanks for accomodating the various change requests here!
What is the purpose of this pull request?
Overview of changes:
Resolves #1605.
Allow using of bootstrap display property
class="d-print-none"to<tabs>,<tab-group>and<tab>which will exclude the entire tab, tab group or single tab respectively during printing. Remove usages of!importantfrom these components to allow for overriding.Anything you'd like to highlight / discuss:
NILTesting instructions:
d-print-noneclassThird tabandExample Tab Groupshould be excluded from the print previewProposed commit message: (wrap lines at 72 characters)
Support opt-out of printing certain tabs
Users may want to omit some tabs when printing.
Let's remove usages of !important in tabs so that
users are able to specify which tab components
to omit when printing using bootstrap's display
property.
Checklist: ☑️