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

Issue 1524/text with bullets #1534

Merged
merged 12 commits into from
Sep 20, 2023
Merged

Issue 1524/text with bullets #1534

merged 12 commits into from
Sep 20, 2023

Conversation

rebecarubio
Copy link
Contributor

@rebecarubio rebecarubio commented Sep 13, 2023

Description

This PR updates the dependencies slate and slate-react that handles the text editor. It also fixes a text editor bugs reg regarding an error and the possibility to remove bullet item when it's on the root of the text editor.

Screenshots

Video.sin.titulo.Hecho.con.Clipchamp.8.mp4
Video.sin.titulo.Hecho.con.Clipchamp.10.mp4

Changes

  • Upgrades slate dependency to version 0.94.1
  • Upgrades slate-react dependency to version 0.98.3
  • Removes an empty space string to fix the type error bug.
  • Adds a fix in editor function called removeBackwards to allow the user to remove a missing bullet point in the root of editor.
  • Trims the instructions string to avoid unnecessary whitespaces.
  • Adds a check in ZUITextEditor useEffect to avoid unnecessary re-renders.

Notes to reviewer

None

Related issues

Resolves #1524

Copy link
Contributor

@kaulfield23 kaulfield23 left a comment

Choose a reason for hiding this comment

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

I just tested it, and it works well! error 1 is fixed but error 2 is still there, the error 2 is about a bullet that never disappears when you revert the text.
Try this

  1. write a word with a bullet and save it. For example,
  • hello
  1. try to give a change to that word, then you will see revert message next to the button, don't save it, but revert it.
  2. You will see the text you wrote with a bullet in number 1.
  3. try to remove a bullet by using slate and save it. For example,

hello

  1. refresh the page
  2. you will see that bullet is still there and impossible to remove it now

If you are unsure about error 2, let me know! I can show you also!

@kaulfield23 kaulfield23 self-assigned this Sep 14, 2023
@rebecarubio
Copy link
Contributor Author

@kaulfield23 I couldn't fix the bug in Firefox so I will maintain that issue documented here #1540. Me and @richardolsson did some more improvements to the Text Editor performance so the PR is again ready to be reviewed.

@kaulfield23
Copy link
Contributor

kaulfield23 commented Sep 19, 2023

Nice!!😄 I really like the improvements! feels so good to be able to remove bullets! But I can't fully understand codes here so I want @richardolsson to double check it!

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I have a question below. Regardless of the answer, I think this looks fine. @kaulfield23 if you're happy with how it works, I give you green light to merge!

Comment on lines +231 to +235
if (node && Object.prototype.hasOwnProperty.call(node[0], 'children')) {
if (
'children' in node[0].children[0] &&
Object.prototype.hasOwnProperty.call(node[0].children[0], 'children')
) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessarily complex to me. I realize you probably found this solution in the Slate issue tracker, but I'm curious if you know the reason for using hasOwnProperty.call(node[0]… instead of node[0]?.hasOwnProperty(…, etc?

Copy link
Contributor Author

@rebecarubio rebecarubio Sep 20, 2023

Choose a reason for hiding this comment

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

Yes, the reason I was implemented so complex it's because Ancestor and the following types are a mixed of types. The reason to use hasOwnProperty.call it's to avoid a lint error. If I use node[0]?.hasOwnProperty(…, I get the error Do not access Object.prototype method 'hasOwnProperty' from target object.

@rebecarubio rebecarubio merged commit 9515e0d into main Sep 20, 2023
@rebecarubio rebecarubio deleted the issue-1524/text-with-bullets branch September 20, 2023 08:32
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.

Texts with bullets in slate occurs error / not working as expected
3 participants