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

Feat(blocks): move blocks #18852

Merged
merged 58 commits into from
Dec 12, 2023
Merged

Feat(blocks): move blocks #18852

merged 58 commits into from
Dec 12, 2023

Conversation

madhurisandbhor
Copy link
Contributor

@madhurisandbhor madhurisandbhor commented Nov 20, 2023

What does it do?

  • Adds the ability to move the blocks around in editor.
  • Blocks and list-items are draggable, list block, link is not draggable.
  • Keyboard shortcut Cmd/Ctrl + Shift + Up/Down ( referenced from notion) added to move blocks.
  • Quote and list styles updated as per design

useDragAndDrop hook Changes :

  1. Hook is updated to accept indices as Array<number>, to support nested list items path, for eg: [1, 0] or for list-item path [1, 0, 0] and handled it separately.
  2. Default functionality of moving items onMoveItem() updated and made it optional as for Blocks this needs to be handled onDropItem().

How to test it?

On hover user should be able to see drag option and be able to drag block and list-items. While dragging user will see a blue line drop-placeholder on droppable area. On drop, item will be moved to respective position specified by drop placeholder.

Design

Related issue(s)/PR(s)

Previous PR : #18690

… is not a number but number[], wrapped renderElement with Dnd component to have ability to move blocks around in editor
…chy of nodes, cmd/ctrl + shift + up/down arrow to move blocks around using keyboard, set live text on moving blocks for accessibility
@madhurisandbhor madhurisandbhor added source: core:content-manager Source is core/content-manager package pr: feature This PR adds a new feature labels Nov 20, 2023
@madhurisandbhor madhurisandbhor self-assigned this Nov 20, 2023
@madhurisandbhor madhurisandbhor marked this pull request as ready for review November 20, 2023 14:54
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

You've changed this hook and you've not updated the contributor documentation.

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

Super excited to try it but I have a TS error when building strapi. It looks like it happens in the github actions too

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

I think your comments in the drag and drop hook need to be more & clearer because "node hierarchy" in the context of that hook doesn't mean much...

Let's continue to discuss the API changes there too 🙂

Copy link
Contributor

github-actions bot commented Nov 21, 2023

Size Change: +2.06 kB (0%)

Total Size: 1.25 MB

Filename Size Change
examples/getstarted/build/main.********.js 1.24 MB +2.06 kB (0%)
ℹ️ View Unchanged
Filename Size Change
examples/getstarted/build/bb4d0d527bdfb161bc5a.svg 2.33 kB 0 B
examples/getstarted/build/index.html 611 B +2 B (0%)
examples/getstarted/build/runtime~main.********.js 4.19 kB -1 B (0%)

compressed-size-action

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

Good idea to use the context 👍

I still need to test it

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

The handle alignment is still odd, but I don't have reordering bugs anymore 💥

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

Just 2 things:

  • I think we need handle margin top for quotes too, it's not aligned with the first line of content
  • I'm noticing some input lag now. Do you know if there's anything in the last few commits that could have impacted performance?

@joshuaellis
Copy link
Member

I'm noticing some input lag now. Do you know if there's anything in the last few commits that could have impacted performance?

You're using a lot of props in styled-components, i'd see if it's recreating too many styles when you're dragging. I've had this before when I was passing top/left for absolute positioning on drag/animation it would create a new class per variable combo passed which is very costly.

@remidej
Copy link
Contributor

remidej commented Dec 8, 2023

Interesting @joshuaellis thanks. But do you think it would impact the lag when I'm just typing text? Not dragging blocks

@joshuaellis
Copy link
Member

Interesting @joshuaellis thanks. But do you think it would impact the lag when I'm just typing text? Not dragging blocks

Looking at the things we've changed here, I wouldn't rule it out and still investigate it because it's easy to see – iirc in the devtools the styles tag will flash to show it's updating, I think. If it's not that then it's of course a bit more complicated to understand!!

But you're totally right to flag input lag, can't be serving users a laggy blocks editor lol

@madhurisandbhor
Copy link
Contributor Author

madhurisandbhor commented Dec 8, 2023

Looking at the things we've changed here, I wouldn't rule it out and still investigate it because it's easy to see – iirc in the devtools the styles tag will flash to show it's updating, I think. If it's not that then it's of course a bit more complicated to understand!!

But you're totally right to flag input lag, can't be serving users a laggy blocks editor lol

@joshuaellis @remidej thanks for your inputs, I checked lagging issue but to me it doesn't seem to be blocking, as its very barely noticeable. There is no issue of styles applied as while typing nothing is getting updated except the element itself.
@remidej Could you please log this bug in jira with video showcasing the lag, thanks

@joshuaellis
Copy link
Member

Does this feature need re-QA'ing before merge since we've had lots of changes since the last one @madhurisandbhor?

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

I can't manage to reproduce the lag I had yesterday, so I think we can move on 👍

@madhurisandbhor
Copy link
Contributor Author

@joshuaellis I think me and Rémi did most of testing of the bugs reported so not really required, it's all good now.

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

Unfortunately I'm seeing the input lag again (not sure why I couldn't on Friday). This time I made a Loom recording to show it better: https://www.loom.com/share/f264942f381040aaa75e95003ec23ffb?sid=1f09c45c-1caa-4312-97f1-b7378818d82f

I think this is something we need to fix. @joshuaellis would you maybe have some ideas?

@madhurisandbhor
Copy link
Contributor Author

@remidej thanks for the recording, But this is clearly somthing else as long_text, rich_text also have the same issue while typing, I am not able check profiler as its also getting stuck on my device recording this issue.

@remidej
Copy link
Contributor

remidej commented Dec 11, 2023

@madhurisandbhor you're right I'm getting it on rich text too, even when I delete the blocks field. So it looks like the issue could come from something else in the develop branch, which would explain why it appeared suddenly in your branch. I'll check after lunch if it happens on main too to be sure.

@remidej
Copy link
Contributor

remidej commented Dec 11, 2023

So actually the bad performance only happens with the --watch-admin flag. It only exists since last week, and does not happen on main. I'll check with the rest of the team to find the source, so let's ignore it in this PR

Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Great work @madhurisandbhor you overcame a lot of challenges in this feature 👏🏼

@madhurisandbhor madhurisandbhor merged commit 1780121 into develop Dec 12, 2023
41 of 47 checks passed
@madhurisandbhor madhurisandbhor deleted the feat/move-blocks-ts branch December 12, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: feature This PR adds a new feature source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants