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

Prepending card if added at the beginning #7

Closed
wants to merge 2 commits into from

Conversation

adelura
Copy link

@adelura adelura commented Apr 5, 2018

Thank you for such a nice wrapper for swing library! It works well. Therefore I found some small issue.

When adding a card to stack swing expose an option to prepend one. Currently, your wrapper allows only appending cards. It's not always the case. There is a scenario when the card is added at the beginning. To handle that, I check if the index of an added card is at 0. It means that card should be prepended.

Also, I removed this.card field because it's not used, and I was confused initially why we need this. I can remove that change if you want - I don't mind.

@adelura adelura changed the title Prepending card if added ad the beginning Prepending card if added at the beginning Apr 5, 2018
@goweiwen
Copy link
Owner

goweiwen commented Apr 6, 2018

Thanks for this, I'll look into it later today or over the weekend.

@adelura
Copy link
Author

adelura commented Apr 6, 2018

I didn't tested it, but there should be still issue for some cases. For example if you add two cards at the same time, then only one of those will have index 0, so one will be added at the beginning, and the other at the end, because the further will be at index 1.

So what we should check instead is whether index of added card is lower that amount of added cards.

For example when there are two cards added, then indexes will be as follows: 0 and 1. Both of those are lower than amount of added cards (2) so those will be prepended.

As the are no automated test in this repository I prefer not to mess much with so I tried to keep change as small as possible.

@goweiwen
Copy link
Owner

goweiwen commented Apr 7, 2018

Ah, I didn't know this was how Swing was implemented. This doesn't solve the issue of when we add a card in the middle of the stack, right? Do you think there's a more elegant way to do this? Otherwise, I'll test this out later and get back to you.

Yes, I don't think the cards array is used anymore. Do you mind making a separate PR to solve just that issue?

@adelura
Copy link
Author

adelura commented Apr 7, 2018

This doesn't solve the issue of when we add a card in the middle of the stack, right?

I think that swing doesn't support such case, you can either append or prepend card. And I think that it's okay, as I can't imagine scenario where someone would like to add card at the beginning.

My case is that I have only three cards visible at the time, so once first one is thrown away then I predend another one so I still have three.

Do you mind making a separate PR to solve just that issue?

I would first add tests for that repository, because It's quite dangerous to change anything, as I never know if I introduced some regression by my change.

@goweiwen
Copy link
Owner

goweiwen commented Apr 9, 2018

I think I'm okay with this implementation if there's a note in the README about how we don't support adding elements in the middle of the stack. If you want to add some documentation, I'll merge it in as well. Otherwise, I'll add it in before the next release.

I wanted it as a separate PR because I didn't want unrelated changes in this PR. After some manual tests, I found that the cards variable is exposed and is used by the example, so it should not be removed. Please revert that change.

Otherwise, looks good to me.

@adelura
Copy link
Author

adelura commented Apr 9, 2018

I will leave documentation up to you as we probably might have different approach to it. In general a swing library doesn't support adding card in the middle, so I can't imagine that wrapper might do.

I reverted the changes so card field is available again.

Cheers.

@adelura adelura closed this Aug 5, 2018
@adelura
Copy link
Author

adelura commented Aug 5, 2018

Closing as no response from you. I'm gonna release the fork: https://github.com/adelura/vue-swing2

@goweiwen
Copy link
Owner

I'll merge your changes separately and release it in the next version since the branch has changed.

Thanks for contributing!

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.

2 participants