-
-
Notifications
You must be signed in to change notification settings - Fork 148
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 direction grid #560
Add direction grid #560
Conversation
@NullVoxPopuli can you make a rerun of test? Local it is passing without any issue and also same changes on my repo are getting a green ci: mkszepp#1 |
@NullVoxPopuli do you have any plan when we can get this feature? |
Sorry, my email situation is a bit wonky atm <3 feature looks good -- gonna review the code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, but can we get some automated tests added? thanks!!
@NullVoxPopuli test were now added... while adding tests i have discovered two bugs, but now they are fixed |
It looks like the test fails, cause a timing... locally i have got also sometimes a fail... maybe we need to add a short delay in |
seems like things are failing pretty regularly -- def can't merge without resolving 🤔 Do you think a test waiter needs to be added so that the waiter system "just works", and consumers of this library then also don't need to worry about adding random waiting in their tests? |
agree, atm its not mergable, beacause tests do fail... i will look to find a fix... maybe |
@NullVoxPopuli i have left some comments... all test should now pass (without retries 😅) While adding this feature i was running in errors, which are still present in current version, but tests were never failed beacuse the lists were always small. There was also never waited if all transition from items were completed, by using transition time with 1 secound + you will get crazy animations 😂. With my fix now we do wait since everything is completed. |
// In test-app it looks like there is a side-effect when we activate also for direction vertical. | ||
// If any user will anytime report a jumping in vertical direction, we should activate for every direction and fix our test-app | ||
if (this.direction === 'grid') { | ||
height += parseFloat(elStyles.marginTop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked shortly in issues.. i think generally adding this could be fix this issue #520, but this should be done in next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fractional pixels can be a doozy!
This feature allows users to create a sortable list that works with a grid. Items can be moved up, down, right, or left within the same list.
Motivation?
We are currently using ember-drag-sort, which supports this feature. Unfortunately, the addon uses classic components, and updating it to the latest edition would require significant effort. Additionally, it is not as flexible as this addon. The drag-and-drop animation in ember-drag-sort is a simple arrow with no options for customization.
Implementing this feature in
ember-sortable
will help us transition away from ember-drag-sort. This update might also encourage others still using the previous addon to switch to this one.