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

feat: add FoldingHeading extension #314

Merged
merged 2 commits into from
Aug 1, 2024
Merged

feat: add FoldingHeading extension #314

merged 2 commits into from
Aug 1, 2024

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.

@@ -17,6 +17,8 @@
"cut": "Cut",
"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?

@@ -7,5 +7,5 @@ export const YfmHeadingAttr = {
Level: headingLevelAttr,
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
3 checks passed
@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.

3 participants