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

Improvements and bugs fixed in Tabs component #38

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

guvial
Copy link

@guvial guvial commented Dec 9, 2018

Hi,
Thanks again for your great work on this lib.
I have used the Tabs component in my project and I faced many issues and limitations using it (mostly reactivity issues).
I wanted to share with the community my improvements / bugs fixed. I hope that you will like it.
Thanks in advance for your code review.
Cheers,
Guillaume

@damienix
Copy link
Contributor

I'm happy it helps you :)

Nice work, I'll add my comments in the review itself...

Copy link
Contributor

@damienix damienix left a comment

Choose a reason for hiding this comment

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

In general, there are some good changes in the code but requires some polishing, I'm happy to make another round of comments after you fix current issues :) 👍

@guvial
Copy link
Author

guvial commented Dec 27, 2018

Thanks a lot for your review, very useful.
I have tried to fix all issues in my last commit.
Let me know if everything is now ok for merging.
I am also relatively new to github forking and pull requests.
I hope that I have done things the right way.
Have a nice day and I wish you a very good 2019 year! ;-)
Cheers,
Guillaume

@guvial
Copy link
Author

guvial commented Jan 30, 2019

Hi Damien,
I hope that I find you well.
Could you find some time to review my updated pull request?
Thanks in advance for your feedbacks,
Cheers,
Guillaume

@damienix
Copy link
Contributor

damienix commented Feb 1, 2019

Hi, sorry for the silence. I didn't have too much spare time recently, but still keeping this PR on my todo list. I'll do my best to review this during this weekend.

damienix added a commit that referenced this pull request Feb 3, 2019
@damienix
Copy link
Contributor

damienix commented Feb 3, 2019

Hi @guvial. To speed things up I've added my changes on top or yours. In summary:

  • I reverted dist files that you have removed - they need to be in the repo, they are just commited on a release
  • I've reworked tab selection to work based on the reactive syncable property rather than imperative function call
  • I've reworked internals to "be more reactive"
  • Fixed a bug when tab selection wouldn't update properly when the same amount were added and removed at the same time

Please let me know if I this version works properly for your case - I did a lot of changes so I might have introduced some new bugs. I've pushed the code here
https://github.com/spartez/vue-aui/tree/pr-38-tabs-fixes.

@Mihailoff
Copy link

Hi, any chance to get this merged?

@damienix
Copy link
Contributor

I'm sorry but I'm no longer with the company nor have access to the repository. You need to find another maintainer from Spartez :(

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.

3 participants