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

[lexical-react][lexical-playground] sync draggable block plugin to www #6397

Merged
merged 13 commits into from
Jul 17, 2024

Conversation

potatowagon
Copy link
Contributor

@potatowagon potatowagon commented Jul 12, 2024

Description

  • abstract out the ui components from draggable block plugin so we can customise the ui for menu and target line

  • currently sticking with original implementation which depends on the ui component to be a html element to be compatible with ref. will explore (in another PR) if its possible to make this more flexible so we can pass in any react component without having to maintain the ref

Closes #

Test plan

Before

Draggable block plugin works

After

Screen.Recording.2024-07-16.at.6.17.17.PM.mov

no regressions, draggable block plugin still works

Copy link

vercel bot commented Jul 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 4:25am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 4:25am

@potatowagon potatowagon marked this pull request as draft July 12, 2024 09:47
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2024
@potatowagon potatowagon changed the title abstract display components sync draggable block plugin to www Jul 12, 2024
Copy link

github-actions bot commented Jul 12, 2024

size-limit report 📦

Path Size
lexical - cjs 28.75 KB (0%)
lexical - esm 28.61 KB (0%)
@lexical/rich-text - cjs 37.2 KB (-0.07% 🔽)
@lexical/rich-text - esm 28.13 KB (0%)
@lexical/plain-text - cjs 35.81 KB (0%)
@lexical/plain-text - esm 25.48 KB (0%)
@lexical/react - cjs 39.05 KB (0%)
@lexical/react - esm 29.48 KB (0%)

…9382ad6991cd3917e33e9aca:66:6: ERROR - [JSC_BLOCK_SCOPED_DECL_MULTIPLY_DECLARED_ERROR] Block-scoped variable Rect declared more than once. First occurrence: externs.zip//w3c_css.js:858:9

  66| class Rect {
@@ -17,7 +17,7 @@ type ContainsPointReturn = {
};
};

export class Rect {
export class Rectangle {
Copy link
Contributor Author

@potatowagon potatowagon Jul 16, 2024

Choose a reason for hiding this comment

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

rename because w3c bundle has a Rect class already and clash in naming cause compiling the prod build to fail

zurfyx
zurfyx previously approved these changes Jul 16, 2024
Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

LGTM, I would consider suffixing the component to __EXPERIMENTAL while we polish the UI since it will give us more flexibility to change the API

}

export default function DraggableBlockPlugin({
export default function PlaygroundDraggableBlockPlugin({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export default function PlaygroundDraggableBlockPlugin({
export default function DraggableBlockPlugin({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cant rename that to DraggableBlockPlugin as thats the name of the base plugin

Screenshot 2024-07-17 at 12 06 46 PM

@potatowagon
Copy link
Contributor Author

LGTM, I would consider suffixing the component to __EXPERIMENTAL while we polish the UI since it will give us more flexibility to change the API

ok added

@potatowagon potatowagon added this pull request to the merge queue Jul 17, 2024
@potatowagon
Copy link
Contributor Author

failure from flaky test

Merged via the queue into main with commit bd26794 Jul 17, 2024
41 of 42 checks passed
@potatowagon potatowagon deleted the sync-plugin branch July 26, 2024 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants