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

Add class method acts_as_list_top as reader for configured top_of_list #213

Merged
merged 1 commit into from Jul 15, 2016

Conversation

krzysiek1507
Copy link
Contributor

No description provided.

@brendon
Copy link
Owner

brendon commented Jul 12, 2016

Hi @krzysiek1507, could you give a bit of a use case?

Also, this change seems to be randomly breaking tests. The test suite is run in a random order each time so your additional class method is probably interfering in some way.

@krzysiek1507
Copy link
Contributor Author

krzysiek1507 commented Jul 12, 2016

Hi @brendon I need access to top_of_list when I want to reposition items in loop using set_list_position. I can get this from an instance, but I have to create a new object so this is not the best way.

Maybe we should add top_of_list to this new_position? There is no sens to set position to 0 if top_of_list is set to 100. Am I correct?

No, something like:

new_position = new_position < acts_as_list_top ? new_position + acts_as_list_top : new_position

@krzysiek1507 krzysiek1507 force-pushed the add-class-method-for-list-top branch from 0f3ef57 to c19e40c Compare July 12, 2016 23:01
@krzysiek1507 krzysiek1507 force-pushed the add-class-method-for-list-top branch from c19e40c to 8a520a8 Compare July 12, 2016 23:05
@krzysiek1507
Copy link
Contributor Author

@brendon what do you think?

@brendon
Copy link
Owner

brendon commented Jul 13, 2016

Hi @krzysiek1507, I'm not keen to change the way that method works. That'd be an unexpected change for a lot of people who may be relying on the fact that it just assigns a position without question.

In regards to your modification, I'm at a loss as to why the test cases are now all passing. What did you change to fix it? I just want to make sure it's not just a fluke run that worked.

@krzysiek1507
Copy link
Contributor Author

@brendon I've added previously missed setup method.

@brendon
Copy link
Owner

brendon commented Jul 14, 2016

Thanks @krzysiek1507,

@swanandp and @fabn, are you ok with this addition?

@fabn
Copy link
Collaborator

fabn commented Jul 14, 2016

For me it can be useful 👍

@brendon
Copy link
Owner

brendon commented Jul 15, 2016

Done :)

@brendon brendon merged commit db0f309 into brendon:master Jul 15, 2016
@krzysiek1507
Copy link
Contributor Author

Thanks!

@krzysiek1507 krzysiek1507 deleted the add-class-method-for-list-top branch July 15, 2016 08:15
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

3 participants