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

ENH MutliLinkField sorting #166

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Jan 10, 2024

Issue #139

react-dnd -> dndkit

Most recent version of react-dnd is 16 which was released in Aug 2022 (we're still using version 5 on elemental), it failed to work at all in our setup

  • I followed the basic "quick start" instructions and got a console warning
  • Jest failed to work, complained about an 'Unexected export token' in a react-dnd file. Apparently it uses the wrong JS syntax for our setup.
  • Looking around some github comments it seems like the export issue started with version 16, it wasn't a problem with version 15

I've opted to use https://docs.dndkit.com/ instead which seems to work well and had its last release in Nov 2023

Screen jumping on sort

The screen jumps on sort, though it also does this on anything that causes a re-render e.g. link edit. I've created a new issue for this. #169

Hover/focus styling

Styling for the LinkField hover/active/focus states is generally poor, I've created a new issue to tidy this up #168

Design review

Design team has reviewed and approved with understanding the screen jumping and hover/focus styling will be handled separately

@emteknetnz emteknetnz force-pushed the pulls/4/sorting branch 6 times, most recently from 4c9c78d to 36cca91 Compare January 12, 2024 02:56
@emteknetnz emteknetnz marked this pull request as ready for review January 12, 2024 02:56
@emteknetnz emteknetnz force-pushed the pulls/4/sorting branch 3 times, most recently from 7b79a59 to 3af272f Compare January 15, 2024 02:56
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

The bottom border of the last item in the list is missing:
Before:
image

This PR:
image

@GuySartorelli
Copy link
Member

I took this after rebasing so I don't have the scroll popping issue.

Screencast.from.16-01-24.10.00.44.webm

There are two issues here.

Flickering

I think we shouldn't bother trying to show the loading component here because that flickering is just very visually distracting and doesn't have any clear benefit.

At some point that decision looks like it was made for elemental as well, because elemental doesn't show the loading component when sorting.

Briefly rendered in the wrong order

Very briefly after letting go of the item I'm dragging, it pops back into its original position, and then pops into its new position after loading.

We should immediately render it in its new position after dropping, and only pop it back to its original position if something goes wrong with the network request to save the new sort order.

client/src/components/LinkField/LinkField.js Outdated Show resolved Hide resolved
client/src/components/LinkField/LinkField.js Outdated Show resolved Hide resolved
src/Controllers/LinkFieldController.php Show resolved Hide resolved
src/Controllers/LinkFieldController.php Show resolved Hide resolved
src/Controllers/LinkFieldController.php Outdated Show resolved Hide resolved
src/Controllers/LinkFieldController.php Outdated Show resolved Hide resolved
src/Models/Link.php Outdated Show resolved Hide resolved
src/Models/Link.php Outdated Show resolved Hide resolved
src/Models/Link.php Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Also needs rebase please

@emteknetnz emteknetnz force-pushed the pulls/4/sorting branch 4 times, most recently from 6b5f197 to 9d53967 Compare January 17, 2024 00:10
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

The actual sorting works really well now - I like that it's constrained to the parent and the vertical axis.

But now when I add new links, they don't appear until I refresh the page:
Screencast from 17-01-24 15:03:47.webm

src/Controllers/LinkFieldController.php Outdated Show resolved Hide resolved
src/Controllers/LinkFieldController.php Outdated Show resolved Hide resolved
client/src/components/LinkPicker/LinkPicker.scss Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks great, works well locally.

@GuySartorelli GuySartorelli merged commit 663a13f into silverstripe:4 Jan 17, 2024
10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4/sorting branch January 17, 2024 04: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.

2 participants