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(tabs): vertical tabs + new props for adding classes to inner elements #1362

Merged
merged 17 commits into from Nov 20, 2017

Conversation

Projects
None yet
5 participants
@tmorehouse
Member

tmorehouse commented Nov 18, 2017

Adds support for vertical tabs via the vertical prop

Also adds the following new props:

  • container-class added to the .tab-container div
  • nav-class added to the .nav tab controls
  • nav-wrapper-class added to div wrapper around nav tab controls

tmorehouse added some commits Nov 18, 2017

feat(tabs): add props for adding classes to inner elements
Adds the following new props:
- `containerClass` added to the `.tab-container` div
- `navClass` added to the `.nav` tab controls
- `navWrapperClass` (added to div wrapper around nav tab controls
@codecov-io

This comment has been minimized.

codecov-io commented Nov 18, 2017

Codecov Report

Merging #1362 into dev will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev    #1362      +/-   ##
=========================================
+ Coverage   43.1%   43.14%   +0.03%     
=========================================
  Files        130      130              
  Lines       2865     2867       +2     
  Branches     884      885       +1     
=========================================
+ Hits        1235     1237       +2     
  Misses      1245     1245              
  Partials     385      385
Impacted Files Coverage Δ
src/components/tabs/tab.js 66.66% <ø> (ø) ⬆️
src/components/tabs/tabs.js 57.33% <100%> (+1.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f50319e...ce0f8d7. Read the comment docs.

@tmorehouse tmorehouse changed the title from feat(tabs): add props for adding classes to inner elements to feat(tabs): new props for adding classes to inner elements Nov 18, 2017

tmorehouse added some commits Nov 19, 2017

@tmorehouse tmorehouse changed the title from feat(tabs): new props for adding classes to inner elements to feat(tabs): vertical tabs + new props for adding classes to inner elements Nov 19, 2017

@tmorehouse tmorehouse removed the Status: WIP label Nov 19, 2017

@tmorehouse tmorehouse requested review from pi0, alexsasharegan and mosinve Nov 19, 2017

@@ -92,7 +183,7 @@ In some situations, you may want to add classes to the `<li>` (nav-item) and/or
the `title-item-class` prop (for the `<li>` element) or `title-link-class` prop (for the
`<a>` element). Value's can be passed as a string or array of strings.

Note: THe `ative` class is automatically applied to the `<a>` element. You may need to accomodate
Note: THe `ative` class is automatically applied to the active tabs `<a>` element. You may need to accomodate

This comment has been minimized.

@Morgul

Morgul Nov 19, 2017

Two typos: THe and ative.

This comment has been minimized.

@tmorehouse

tmorehouse Nov 19, 2017

Member

Thanks! 👍

This comment has been minimized.

@Morgul

Morgul Nov 19, 2017

No problem! Just happened to notice while looking through the changes. 😄

@pimlie

This comment has been minimized.

Contributor

pimlie commented Nov 19, 2017

Some remarks (some you already mentioned yourself in #558):

  • the vertical tabs are only displayed correctly when using pills, 'normal' tabs are not symmetrical so you need the css from #558 to have correct borders etc
  • the pill'ed bottom tabs are missing a border (probably need to switch class card-header to card-footer)
  • I think adding no-gutters class should be optional, a user might want to (try to) align the columns with a previous row and now we force them to use no-gutters on that row as well
  • In my previous PR you mentioned that for assistive technologies it would be best that in the html structure the tab navs always comes first, thats partially why I used order-12 classes to move the tab navs for position bottom and right. In this PR thats not the case, is that on purpose or an oversight?

Although I am grateful for your assistance with this feature, I feel like we jumped ship a bit as it seems the questions I asked here #558 (comment) are still relevant. To me this PR only confuses the situation a bit more, because I still believe that we should either support bottom / vertical tabs fully including all the required css or if you dont want that for b-tabs we make the move to a separate b-tabs-extra component.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 19, 2017

We're trying to work with the limitations of the existing Bootstrap V4 CSS and using existing styling classes to make it work.

Even in native Bootstrap vertical navs, the do not style the tabs appropriately. They show the regular tab style (as they re just using flex-column class to move them into vertical mode, and not any specific class for navs). Here is what native Bootstrap styling looks like for vertical nav-tabs:
image

Maybe you could open an issue in the twbs/bootstrap repo regarding tab styling in vertical (stacked) mode.

I still agree that the tab controls should be visually moved to the end, rather than physically. We can address that in this PR.

I'll see if using the card-footer class will add the top border when the controls are moved to the bottom.

As for the border when placed on the sides, bootstrap doesn't have any CSS classes for adding the side border (just removing them) unfortunately.

@tmorehouse tmorehouse changed the title from feat(tabs): vertical tabs + new props for adding classes to inner elements to [WIP] feat(tabs): vertical tabs + new props for adding classes to inner elements Nov 19, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 19, 2017

no-gutters removes the padding from the columns and the negative margins from the b-row. the row itself will be aligned with any other row before or after it (even if they don't have the no-gutters set). The columns won't necessarily align (but their edge boundaries will).

Without the no-gutters option, there is overly excessive space between the tabs and the content when in vertical mode (basically double what is expected).

One can always create a custom class which is applied to the b-tabs component, and use that class to target the styling of the inner elements within b-tabs (i.e. changing the Bootstrap default styling of the nav-item in the tab controls).

tmorehouse added some commits Nov 19, 2017

@pimlie

This comment has been minimized.

Contributor

pimlie commented Nov 19, 2017

Bootstrap has removed support for vertical tabs since v3.0 (see their issue 6342) so I dont think opening styling issues about that in the bootstrap repo will be useful. They specifically stated that: You can still use the JavaScript plugin and custom CSS to make left/right tabs, but we will no longer include those in the core.

That leaves us at the main question again, how far do we want to stray away with bootstrap-vue in this case? Bootstrap doesn't support any other tabs then with the navs on top. They dont provide css for tabs on the left, right or even bottom and I am quite sure they never will because that probably doesnt fit in their mobile-first strategy. Again, I dont think we should take the 'easy' route by only partially supporting vertical tabs due to not providing the correct css for all use-cases that the b-tabs component provides.

With regards to no-gutters, I am sure you will understand that I ran into that problem as well with my PR. But I still made it optional as I don't think we should oppose styling options like that on users. Although you are right by saying you can always add a custom class and change the inner elements with that, that's much more difficult then just a no-gutters="false" prop.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 19, 2017

Visually, without the no-gutters class, the layout sucks (especially on narrower screens due to the excessive double padding that occurs from card components). When not used with card integration, the no-gutters class is not applied.

I think what is here in this PR adds enough functionality within the limits of Bootstrap V4's CSS, and provides basic support and accessible markup. Visually, if people want to change how the tabs mode looks in vertical mode, they can provide custom CSS to address it, just as they would have to if using vertical tabs style navs in Bootstrap V4 native. The visual styles of the vertical (and bottom placement) tab style navs, is documented as not being "stylish", however, the pills style works quite well.

There is enough functionality within the component that masks some of the gory tweaks that needs to be done in order to make the controls vertical which normally couldn't be done easily (due to the elements being buried inside a component).

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 19, 2017

Hmm after playing with ordering, it doesn't work well for keyboard users as the visual tab sequence is confusing (if there are elements with tab stops inside the content).

For visually impaired users, while ordering would make better semantic sense (nav controls always on top), we do have ARIA attributes that to relate the content to the elements (navs) that do control the content. So I think reverting back to physical ordering of the navs/content might be best (rather than using flex ordering)

@pimlie

This comment has been minimized.

Contributor

pimlie commented Nov 19, 2017

Although I understand your view about having enough gory tweaks I have to respectfully disagree with your conclusion.

All in all we are providing just half of the solution and are shifting the workload to the users who individually have to repeat the same process again and again. Isn't there a mid-way like including the required css as an extra component or maybe even a separate but official package? Then at least we provide the full solution and maintenance on vertical tabs only has to be done once for the whole user-base.

Maybe this is nitpicking but I think we should strive towards that it works 'well' and not settle for 'quite well' 😄

tmorehouse added some commits Nov 19, 2017

tabs: revert to physical ordering from flex ordering
Keyboard navigation using flex ordering sucks
@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

The real issue is Bootstrap's CSS. We do want to avoid any massive amounts of custom CSS in Bootstrap-Vue, as it can easily conflict with user's custom CSS, or bootstrap V4 custom CSS

Visually I do not find the tab version that bad when in vertical mode. It doesn't look good in non-vertical bottom mode, but that is just a caveat of the base Bootstrap V4 CSS.
OK:
image
Not as OK, but not ugly:
image
Definitely OK:
image
Definitely OK as well:
image

The big issue with custom CSS, when not used to just tweak positioning/margins, temporary fixes for bugs in Bootstrap V4's beta CSS (i.e. form-file), or providing non-bootstrap styles for things like the sorting arrows in b-table, but actually controls border styles and color, is that it ends up being hard coded in the bootstrap-vue.css distribution file. If someone is using a custom themed Bootstrap V4 (which changes the default border colors of say, nav-tabs, then our hard coded CSS overrides some of theirs ending up with wonky looking components, or things like nav-tabs not following their custom theaming 😮

Even if one uses SCSS to define the custom component styles (i.e. making new style for the b-tabs nav items), importing the variables from the Bootstrap V4 SCSS, then end result when Bootstrap-Vue gets built/compiled and distributed is that the SCSS variables are compiled into absolute/static values in the final bootstrap-vue.css dist file,

Importing that bootstrap-vue.css file into someones project may end up with components not matching the styling of the rest of their project. This is why it is best to avoid as much custom CSS as possible.

@tmorehouse tmorehouse changed the title from [WIP] feat(tabs): vertical tabs + new props for adding classes to inner elements to feat(tabs): vertical tabs + new props for adding classes to inner elements Nov 20, 2017

@tmorehouse tmorehouse removed the Status: WIP label Nov 20, 2017

@pimlie

This comment has been minimized.

Contributor

pimlie commented Nov 20, 2017

This is why it is best to avoid as much custom CSS as possible

Sure, but I still dont agree with your conclusion in this case as the bulk of the users will probably not use theming anyway. And even if they would there is no difference for them to just not include the bootstrap-vue(.extra?).css. I think the problem cases you are describing as an argument against adding the custom CSS actually are advanced edge-cases for which we can also easily provide a solution by adding all custom css in scss file ourselves. The fact that some custom 3rd party bootstrap theme doesnt include styling for vertical tabs is evidently and really cant be used as an argument against including CSS here.

The factual issue at hand is that if we have made the decision to support a feature that is not supported by bootstrap we should support it all the way so that at least the basics are covered. Otherwise you are just saying Well you can use that but it was too difficult or too much work for us to implement fully and because we only see problems and no solutions you have to do the rest yourself

Sorry, I really appreciate your hard work but I just feel very strongly about half-assing stuff like this. The argument against adding custom CSS should be made before adding a feature and not after adding a feature 😞

So eh, my last proposal before I will stop this discussion and mock and curse in silence 😉 What about taking an example from nuxt and create a bootstrap-vue community repo / organisation where stuff like this can be added? That way you don't have to deal with the css and it will become much easier to list/share custom/extra components that should work nicely with bootstrap-vue but are supported by the community instead.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 20, 2017

Vertical (stacked) navs are supported by Bootstrap V4:

Example from Bootstrap V4 official docs:

image

They just don't provide any special styling for the tabs flavor, which appears to be the main issue you are concerned with.

And i disagree with your consideration of it being half-assed, as it works just like the bootstrap native version described in their docs (with the exception they do not show bottom or right alignment on the tab controls).

@pimlie

This comment has been minimized.

Contributor

pimlie commented Nov 20, 2017

That was indeed maybe a bit harsh statement of me, sorry about that. So eh, case closed what I am concerned.

And thanks for at least taking the time to explain your point of view, whether we agree or not 👍

@tmorehouse tmorehouse merged commit 51d0e03 into dev Nov 20, 2017

2 checks passed

License Compliance License checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse deleted the feat/tabs branch Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment