Skip to content

Conversation

@d3m1d0v
Copy link
Member

@d3m1d0v d3m1d0v commented Jul 30, 2024

No description provided.

@d3m1d0v d3m1d0v requested a review from makhnatkin July 30, 2024 17:09
@d3m1d0v
Copy link
Member Author

d3m1d0v commented Jul 30, 2024

TODO: add docs how to connect FoldingHeading extension

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

"emoji": "Emoji",
"file": "File",
"folding-heading": "Collapsed section",
"folding-heading_hint": "The text under the heading can be collapsed or expanded.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a dot at the end?

Id: 'id',
DataLine: 'data-line',
Folding: 'folding',
Folding: 'folded',
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems unusual that the term "folded" has been used in conjunction with the key Folding. Is this change necessary? Would it be possible to use "folding", to maintain consistency with the rest of the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

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

"folded" explains the meaning better. That can be true if heading is folded (section is collapsed). False, if heading is open. And null if it is a common heading.

Field "Folding" remains the same for backward compatibility.

decorations.push(
Decoration.node(item.from, item.to, {
nodeName: 'div',
class: 'pm-h-folding-label',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name of the class is not clear. Maybe you should not use abbreviations? For example: pm-folding-heading-label

Also do we need to put pm in front of the class name in our code? I thought pm is the prefix that prosemirror puts in its own code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also do we need to put pm in front of the class name in our code? I thought pm is the prefix that prosemirror puts in its own code.

prosemirror uses prefix ProseMirror- for its own classes.
I don't see anything wrong with using the prefix pm- to denote entities that can only be inside the wysiwyg editor.

@makhnatkin
Copy link
Collaborator

Screenshot 2024-07-31 at 14 01 48

The separator was inserted in the wrong place

TokenType.ContentClose,
];

export const skipSectionsPlugin: PluginSimple = (md) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add usage comment plz

return buildDecosSet(tr.doc);
}

if (!tr.docChanged) return prev.map(tr.mapping, tr.doc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use one if
for return prev.map(tr.mapping, tr.doc);

Copy link
Collaborator

Choose a reason for hiding this comment

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

add comment pzl about optimization

@d3m1d0v d3m1d0v marked this pull request as ready for review July 31, 2024 15:18
@d3m1d0v d3m1d0v merged commit b904704 into main Aug 1, 2024
@d3m1d0v d3m1d0v deleted the hfolding branch August 1, 2024 08:15
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.

4 participants