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.

Sign up for GitHub

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

Jump to bottom

Editor fast follow #653

Merged
merged 8 commits into from
Oct 5, 2017
Merged

Editor fast follow #653

merged 8 commits into from
Oct 5, 2017

Conversation

erquhart
Copy link
Contributor

@erquhart erquhart commented Oct 4, 2017 •
edited
Loading

Bugfixes and improvements for the new Slate editor. Continuation of #629. Closes #575.

NOTE: merge after #638

@erquhart erquhart changed the title Mo editor fast follow Editor fast follow Oct 4, 2017
@erquhart erquhart force-pushed the mo-editor-fast-follow branch from eacfdcf to 3fbe45e Compare October 4, 2017 20:09
tech4him1
tech4him1 reviewed Oct 4, 2017
View reviewed changes
Copy link
Contributor

@tech4him1 tech4him1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If I highlight text in a code block (while in visual mode), and click the italic or bold button, it locks up the whole editor (I cannot change anything), and the "H1" button is highlighted.

src/components/Widgets/Markdown/serializers/index.js
*/
const escapedMdast = unified()
const processedMdast = unified()
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 This makes a lot more sense.

src/components/Widgets/Markdown/serializers/index.js
/**
* Return markdown with trailing whitespace removed.
*/
return trimEnd(markdown);
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes at least two annoying bugs I've been having, thank you!

src/components/Widgets/Markdown/serializers/remarkEscapeMarkdownEntities.js
@@ -248,6 +248,12 @@ function escape(delim) {
*/
export default function remarkEscapeMarkdownEntities() {
const transform = (node, index) => {
/**
* Shortcode nodes will intentionally inject markdown entities in text node
* children not be escaped.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a typo here, or do I just not understand what it means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that's a typo.

@erquhart erquhart force-pushed the mo-editor-fast-follow branch from 3fbe45e to 4e34541 Compare October 4, 2017 22:56
@tech4him1
Copy link
Contributor

tech4him1 commented Oct 4, 2017 •
edited
Loading

If an undo (Ctrl+Z) causes a block to be removed (shift to another type of block), the editor loses focus.

Steps to Reproduce:

  1. Use one of the files in the example (I am using post 10).
  2. Add a new line of text in the visual editor.
  3. Convert it to a different kind of block (header, code block, etc.).
  4. Do an undo (Ctrl+Z). The editor will lose focus.

This happens on redos as well (Ctrl+Y).

@tech4him1
Copy link
Contributor

tech4him1 commented Oct 4, 2017 •
edited
Loading

A bug related to the one above: Moved to #655.

@erquhart
Copy link
Contributor Author

erquhart commented Oct 4, 2017

Yeah, this is actually similar to the first bug you found - replacing the node that the cursor is in is the cause of all of them.

@erquhart erquhart force-pushed the mo-editor-fast-follow branch 2 times, most recently from 0522528 to b1847c8 Compare October 5, 2017 01:28
Benaiah
Benaiah suggested changes Oct 5, 2017
View reviewed changes
Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

A couple (minor) style points and a weird call to flatMap - other than that this LGTM.

src/components/Widgets/Markdown/serializers/slateRemark.js Outdated
@@ -435,7 +435,9 @@ function convertNode(node, children, shortcodePlugins) {
* value and the "lang" data property to the new MDAST node.
*/
case 'code': {
const value = get(node.nodes, [0, 'text']);
const value = flatMap(node.nodes, child => {
return flatMap(child.ranges, 'text');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think a string is a valid iteratee for flatMap, unless I'm missing something here. The result of this will be child.ranges but with every value undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ranges is always an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was misunderstanding was that a string passed to the iteratee argument of flatMap will be treated as an argument to an implicit properties call, which makes this equivalent to something like flatMap(get('text'), child.ranges) in lodash/fp or ramda. LGTM

src/components/Widgets/Markdown/MarkdownControl/VisualEditor/keys.js Outdated
@@ -6,7 +6,7 @@ function onKeyDown(e, data, change) {
const createDefaultBlock = () => {
return Block.create({
type: 'paragraph',
nodes: [Text.createFromString('')]
nodes: [Text.create('')]
Copy link
Contributor

Choose a reason for hiding this comment

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

No trailing comma; this is a linter regression.

@erquhart erquhart force-pushed the mo-editor-fast-follow branch from b1847c8 to b96b7f8 Compare October 5, 2017 16:54
@erquhart erquhart force-pushed the mo-editor-fast-follow branch from b96b7f8 to 65c9e9d Compare October 5, 2017 16:58
@Benaiah Benaiah merged commit c132df9 into master Oct 5, 2017
@tech4him1 tech4him1 deleted the mo-editor-fast-follow branch October 5, 2017 17:37
@martinjagodic martinjagodic mentioned this pull request Aug 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tech4him1 tech4him1 tech4him1 left review comments

@Benaiah Benaiah Benaiah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Markdown editor fast fixes - 0.5.0 beta
3 participants
@erquhart @tech4him1 @Benaiah