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

Replace draft-js with a textarea #568

Closed
wants to merge 4 commits into from
Closed

Replace draft-js with a textarea #568

wants to merge 4 commits into from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jun 7, 2017

Draft-js was lovely but a performance hog on long notes (notes with
several thousand lines).

This patch brings back the textarea and tries to maintain some control
over the content, adding back the helpers such as list completion.

Problems:

  • state updates are wonky. we need to change the debounced updater because it leaves local state out of sync. this is also a problem in the production version with draft js
  • maybe tabbing with selection starting at end of line doesn't work?
  • search results are not highlighted

@dmsnell
Copy link
Member Author

dmsnell commented Jun 7, 2017

The more I get into this the more I feel like we need a text editor that more intrinsically coupled to the data model. draft-js at least operates on atomic edit operations with descriptions, but the textarea don't care. It gives new states on update but doesn't indicate how the content changed. For large notes this too could lead to big enough performance issues to be difficult. Actually, I'm sure this textarea is leagues better than draft-js was, but it does add additional complexity to the state updates.

I also want to check out the Monaco editor (which powers Visual Studio Code) and see if we can get it look right visually. It's already blazing fast and they have done the heavy-lifting to making large documents scroll and update in controlled manners.

There are approaches that involve splitting the input into separate <p> tags for each line but they additionally require using contentEditable in order to make scrolling work. Some issues revolve around updating the DOM nodes appropriately and not getting lost in the confusion.

Copons
Copons previously requested changes Jul 3, 2017
Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

First thing first: this does not boot with a fresh node_modules folder, since there are a couple of Draft leftovers around:

https://github.com/Automattic/simplenote-electron/blob/master/scss/app.scss#L37

https://github.com/Automattic/simplenote-electron/blob/master/webpack.config.dll.js#L12

(Locally I've also got them in the desktop-build/package.json, but I assume it's automatically generated after the normal package.json on build)

@dmsnell dmsnell force-pushed the remove/draft-js branch from d37b2fa to 121b784 Compare July 4, 2017 15:50
@dmsnell
Copy link
Member Author

dmsnell commented Jul 4, 2017

Thanks @Copons - those references have been removed

@dmsnell
Copy link
Member Author

dmsnell commented Jul 11, 2017

@Copons I think this one is ready. Interested in giving it a go-around @kwight?

In my own tests it appears significantly faster to load very large notes. List autocomplete appears to be working relatively well - I'm not sure how to exhaustively test it. Indent and outdent also appear to be working as I expect them to.

@Copons
Copy link
Contributor

Copons commented Jul 11, 2017

(Sorry for the ugly review dismissal message, didn't think it appeared like that - and it's not editable!)

Anyway, styling problem: the textarea vertical scrollbar.

Preview Edit Edit Small Screen
screen shot 2017-07-11 at 18 06 21 screen shot 2017-07-11 at 18 06 11 screen shot 2017-07-11 at 18 08 58

In Preview mode, the overflowing container is the entire "content section", so the scrollbar is correctly positioned next to the right edge of the screen.
In Edit mode, instead, it's the textarea that overflows, and the scrollbar is instead positioned inside the textarea itself, which is typically quite far from the screen edge.

Ideally, in Edit mode, we'd want for the containers to have no padding and no scrollbars; while applying them to the textarea instead.

.note-detail {
  overflow-y: hidden;
}
.note-detail-textarea {
  max-width: 100%;
  padding: 0;
}

.note-detail-textarea textarea {
  // small screens
  padding: 24px;

  // medium screens
  // up to 780px = max width of textarea (684px) + padding (48px per side)
  padding: 24px 48px;

  // large screens
  // dynamically calculate the horizontal padding
  padding: 24px calc( ( ( 100% - 780px) / 2 ) + 48px );
}

@kwight
Copy link
Contributor

kwight commented Jul 11, 2017

Whenever I stop typing in the textarea, the cursor jumps to the tag section:

jul-11-2017 10-37-28

No other surprises though; also tested list behaviour (thought numbered lists wasn't working, but it seems only bulleted lists are supported?).

@gwwar
Copy link

gwwar commented Sep 1, 2017

@dmsnell is this one ready to look at? Are the problems you listed in the summary something we're ok shipping with or did you need help tracking them down?

Draft-js was lovely but a performance hog on long notes (notes with
several thousand lines).

This patch brings back the `textarea` and tries to maintain some control
over the content, adding back the helpers such as list completion.
@dmsnell
Copy link
Member Author

dmsnell commented Oct 3, 2017

I updated this branch to the latest master but I think I lost some important bits related to focus - stuff that was updated in master since I started working here. Need to resolve that before merging.

@rodrigoi
Copy link
Contributor

@dmsnell do you need any help closing this PR?

dmsnell added a commit that referenced this pull request Jan 15, 2018
We've had a few issues with draft-js including performance conderns.
Recently we've also been wanting to explore Markdown syntax highlighting
and while draft-js is ammenable to that it leaves considerable
implementation details up to us.

In #568 I proposed a change to replace draft-js with a plain old
textarea which greatly improved the performance for large notes but also
had the major drawback that we lost our ability to easily decorate text.

Microsoft's Monaco editor powers Visual Studio code and is incredibly
performant thanks to the fact that they have rebuild all the editor
behaviors in custom ways to handle big files and syntax highlighting.

In this patch we're taking advantage of the work that Microsoft has done
and I'm proposing a switch to Monaco. It includes Markdown syntax
highlighting for free. This comes through a React wrapper around the
editor.

The only real drawback I see here is that Monaco is much larger than the
textarea in terms of raw KB though I'm not sure how it compares to
draft-js. In Electron this isn't too much of a concern but if/when we
push this version of the app out to the web it could come with other
problems. Monaco also doesn't officially support mobile devices either I
think.

**Remaining work**

Monaco already has great auto-indent support and provides many of the
behaviors for us which we were implementing by hand with draft-js or in
the textarea. However, it doesn't provide all such as auto-lists.

There remains some dataflow issues that exist already in the app too but
which aren't solved here. When making changes it's possible to queue up
changes but get updates from the server in the meantime and mess up the
editor.
dmsnell added a commit that referenced this pull request Feb 17, 2018
We've had a few issues with draft-js including performance conderns.
Recently we've also been wanting to explore Markdown syntax highlighting
and while draft-js is ammenable to that it leaves considerable
implementation details up to us.

In #568 I proposed a change to replace draft-js with a plain old
textarea which greatly improved the performance for large notes but also
had the major drawback that we lost our ability to easily decorate text.

Microsoft's Monaco editor powers Visual Studio code and is incredibly
performant thanks to the fact that they have rebuild all the editor
behaviors in custom ways to handle big files and syntax highlighting.

In this patch we're taking advantage of the work that Microsoft has done
and I'm proposing a switch to Monaco. It includes Markdown syntax
highlighting for free. This comes through a React wrapper around the
editor.

The only real drawback I see here is that Monaco is much larger than the
textarea in terms of raw KB though I'm not sure how it compares to
draft-js. In Electron this isn't too much of a concern but if/when we
push this version of the app out to the web it could come with other
problems. Monaco also doesn't officially support mobile devices either I
think.

**Remaining work**

Monaco already has great auto-indent support and provides many of the
behaviors for us which we were implementing by hand with draft-js or in
the textarea. However, it doesn't provide all such as auto-lists.

There remains some dataflow issues that exist already in the app too but
which aren't solved here. When making changes it's possible to queue up
changes but get updates from the server in the meantime and mess up the
editor.
@dmsnell
Copy link
Member Author

dmsnell commented Feb 18, 2018

Closed in favor of #692

@dmsnell dmsnell closed this Feb 18, 2018
@dmsnell dmsnell deleted the remove/draft-js branch February 18, 2018 02:51
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.

5 participants