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 direction grid #560

Merged
merged 21 commits into from
Jul 6, 2024

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Jun 11, 2024

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.

grafik

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 12, 2024

@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

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 17, 2024

@NullVoxPopuli do you have any plan when we can get this feature?

@NullVoxPopuli
Copy link
Contributor

Sorry, my email situation is a bit wonky atm <3 feature looks good -- gonna review the code

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a 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!!

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 19, 2024

@NullVoxPopuli test were now added... while adding tests i have discovered two bugs, but now they are fixed

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 19, 2024

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 reorder test helper, because we are moving 26 items in some seconds.. on my fork PR tests are looking sometimes good (but i have also needed some test retries)

@NullVoxPopuli
Copy link
Contributor

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?

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 19, 2024

agree, atm its not mergable, beacause tests do fail... i will look to find a fix... maybe waitUntil helps me... i will try

@mkszepp
Copy link
Contributor Author

mkszepp commented Jun 20, 2024

@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.
drag has never waited for transitionEnd, it works with 5 elements without any issue, but when we use more (can' t tell you the exact number) this tests do always randomly fail like we have now got in grid.
For consumer apps this change should not be braking

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);
Copy link
Contributor Author

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

Copy link
Contributor

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!

@NullVoxPopuli NullVoxPopuli merged commit 4eaf5ff into adopted-ember-addons:master Jul 6, 2024
17 checks passed
@github-actions github-actions bot mentioned this pull request Jul 6, 2024
@mkszepp mkszepp deleted the add-grid-drag-drop branch July 7, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants