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

Block marks wrapping behaviour #11

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@eshvedai
Copy link

eshvedai commented Jan 8, 2019

Introduce the disableWrapping property in NodeView class that would control the wrapping behaviour of block marks.

Forum discussion

Rendered RFC

@eshvedai eshvedai changed the title RFC for block marks wrapping behaviour Block marks wrapping behaviour Jan 8, 2019

@marijnh

This comment has been minimized.

Copy link
Member

marijnh commented Jan 8, 2019

Have you considered making this part of the mark spec, rather than the node view? Should it also be available in the HTML serializer? (I think so.) Maybe a property like spanning: false makes more sense than disableWrapping, since the wrapping isn't really what this is about—marks always wrap their content, the question is whether the mark node should continue across multiple nodes.

@eshvedai

This comment has been minimized.

Copy link

eshvedai commented Jan 8, 2019

yep, makes sense to make it available in the html serializer, I guess a mark shouldn't require a nodeView for that to work. I'm happy to update my PR with your suggestion 👍

@marijnh

This comment has been minimized.

Copy link
Member

marijnh commented Jan 8, 2019

Great, please do. Another question is whether this should be available for inline nodes—since text nodes are composites without meaningful boundaries (their boundaries are determined by other nodes and marks, not by their content), it might be a bit meaningless to support per-node splitting for inline marks. On the other hand, it might also be harmless. What do you think?

@eshvedai

This comment has been minimized.

Copy link

eshvedai commented Jan 8, 2019

I think its harmless, besides its behind the optional spanning flag, so we're not going to break existing documents by that. I updated the RFC

@marijnh

This comment has been minimized.

Copy link
Member

marijnh commented Jan 9, 2019

Looks good now! Let's leave it sitting for the one-week review period and then continue.

@bradleyayers

This comment has been minimized.

Copy link
Contributor

bradleyayers commented Jan 9, 2019

A few other alternatives that come to mind that I'm curious about the counter arguments for:

  • Have a unique mark attribute for each node
  • For positioning, just apply styles to the block node (can this approach avoid the need for the mark)?

Otherwise it looks good.

@eshvedai

This comment has been minimized.

Copy link

eshvedai commented Jan 9, 2019

That's a fair feedback, Bradley! 👍

Have a unique mark attribute for each node

that implies introducing new attribute in the document model just to fix visual representation, which I didn't feel comfortable doing.

For positioning, just apply styles to the block node (can this approach avoid the need for the mark)?

we went with this approach until we figured out that more and more block nodes needed to have similar behaviour, that's why we extracted that into a block mark.

@marijnh

This comment has been minimized.

Copy link
Member

marijnh commented Jan 10, 2019

that implies introducing new attribute in the document model just to fix visual representation, which I didn't feel comfortable doing.

I agree that solving this with the feature described in this RFC feels less kludgey than adding an attribute.

@bradleyayers

This comment has been minimized.

Copy link
Contributor

bradleyayers commented Jan 10, 2019

Sounds good to me too.

@eshvedai

This comment has been minimized.

Copy link

eshvedai commented Jan 21, 2019

Are we good to proceed with this RFC? what are the next steps?

@marijnh

This comment has been minimized.

Copy link
Member

marijnh commented Jan 21, 2019

Yes, absolutely—I've reviewed the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment