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

Tabs refactoring #182

Merged
merged 10 commits into from Mar 28, 2017
Merged

Tabs refactoring #182

merged 10 commits into from Mar 28, 2017

Conversation

mosinve
Copy link
Member

@mosinve mosinve commented Mar 28, 2017

Hi @pi0, there is my refactor attempt of Tabs. Please check code b4 merge :) I've tested already, but just for sure :)

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

@mosinve Thnks :) Preparing docs. If ESLint fails i'll fix it too

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

Change list :

  • docs page
  • updated example
  • fix bug when we initially set tab to something not 0
  • changed fade to no-fade for easier usage
  • cards integration ability

@mosinve Would you please check changes and also what is your idea changing navStyle to just a pills prop? Is it intended for special usages or we may have only two possible values for that?

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

About navstyle, I left it as it was. In bootstrap there're 2 options: tabs or pills. So this prop intended to handle this.
Quickly looked at changes from phone , and don't understand about cards... Check this today later from pc.

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

Figured about cards - cool feature :)
One little notation: "card?'card-header':null" - {'card-header': card} looks better IMHO, and according Vue's guide it's the same ;)
tab - why i'd prefer v-show instead v-if: with v-if transition between tabs shows with some twitching and this not corrected by changing transition modes, with v-show it doesn't twitching - very smooth.

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

@mosinve I was just used to card? syntax :)) Yep object style is much better.
v-show may help a little. But a real case: we have a blogging platform & a post editor with 3 tabs editor, preview and comments. All of them are heavy enough ckeditor needs a big js chunk, comments are heavy and if we just use v-show we can't lazy load tabs. We may combine both methods with a lazy prop on each tab-item :

<tab-item v-if="localshow || !lazy" v-show="localshow || lazy">```

And about navs if you agree we can make usage more semantic by (at least) providing `pills` prop to easily change style.

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

I like lazy variant - it's more flexible.
Pills prop - i think about it too, so i agree ))

And one more thing, when i tested tabs on local playground, i catch a bug with parent/child assigment. Because of it i moved tabs init from tab (this.$parent) to tabs (this.$slots) - in playground all components are childen of this.vm, but in real life - tab are child of tabs so old version worked well, but gave an error in playground.

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

@mosinve I didn't get your meaning about $slots . Do you mean it won't work outside of playground?

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

slots work everywhere
$parent/$child - not work in playground

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

I think i got it :)) @mosinve Would you please commit final fixes for that? so we merge & release it sooner :)

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

I'll do

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

funny thing:
if I write in tab item
v-if="localActive" v-show="localActive"
transition begin to work smooth ))) even without lazy prop....

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

😂 It seems being related to <transition> wrapper.

@pi0
Copy link
Member

pi0 commented Mar 28, 2017

@mosinve Merge it when done :) We can be ready for next release

@mosinve mosinve merged commit d082935 into bootstrap-vue:master Mar 28, 2017
@pi0
Copy link
Member

pi0 commented Mar 28, 2017

поздравления :)

@mosinve
Copy link
Member Author

mosinve commented Mar 28, 2017

متشکرم ;) Hope google trans make it correct)))))

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.

None yet

2 participants