-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Restore the empty paragraph inserter #45542
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
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.
Size Change: +5.31 kB (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
I don't mind this. I'm always for removing UI and removing redundant stuff but seems useful for a lot of folks so 🤷 |
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.
@youknowriad Thanks for the PR. Reasoning seems simple enough. However, I have a nag.
- I added a paragraph block.
- With the keyboard, I cannot find an add block button for the paragraph block.
How hard would it be to fix this? I am just not a huge fan of introducing regressions even if it worked the same way before. Pushing for improvements as annoying as it becomes. Is this a feature targeted for mouse users?
It's like the block toolbar, you have to shift tab to get to it. I'm not entirely sure that we can do better than that. |
I'd say that personally, I'd remove the button entirely but since people learned it and seem to rely on it (especially mouse users indeed), I'm just willing to restore it as it was. |
The failing e2e tests seem to be related to this change. |
@youknowriad Ah, I remember that now and it is coming back to me in which PR that was removed. Thanks. Yeah, nothing we can really do to improve that flow ATM. |
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.
LGTM.
I think I managed to fix most of the failing E2E tests. The only remaining failure is related to dropping files on an empty paragraph block. I've also noticed that the Inserter popover for an empty paragraph obscures the block and prevents focusing on RichText. So it might be the reason for the remaining test failure as well. Unfortunately, I'm unfamiliar with the Popover component to try and fix the issue with confidence. Cc @talldan, @kevin940726 ScreencastCleanShot.2022-11-08.at.15.26.30.mp4 |
Thanks for the ping! Yeah, I think there's a bug in this PR that's rendering a Popover on top of the empty paragraph block. It's preventing the paragraph block from receiving pointer events, hence also preventing any drag events too. It might have something to do with the |
Agree with @kevin940726's comment. When the
This is a bit messy. Particular usages of This code could probably be tidied up in a separate more risky non-backported PR. I think only children of the |
Thank you, @kevin940726 and @talldan! I tried adding another selector to that rule, but now I cannot click the button to open the empty paragraph inserter 😅 If you think of any simple change we can include in a minor release, please feel free to push it to this branch 🙇 |
I think I fixed. I don't like the CSS hacks I added but can't think of a simpler way. |
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.
Thank you, @youknowriad. This fixed the remaining issue.
I wouldn't say I like these CSS hacks, but this simple solution will work for the minor release.
* Restore the empty paragraph inserter * Try fixing failing e2e tests * Fix pointer events Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
* Restore the empty paragraph inserter * Try fixing failing e2e tests * Fix pointer events Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Fixes #45497
What?
In #40441 we removed the inserter that was showing up at the right of empty paragraphs when focused. The reasoning was that it's a redundant piece of UI. It turns out a lot of people do actually rely on it. So this PR restores it.
Testing Instructions
1- Insert empty paragraph blocks
2- Notice that there's an inserter at the right of these blocks.