Skip to content

Wrap footnote definition in a paragraph#15

Closed
vhf wants to merge 1 commit intosyntax-tree:masterfrom
vhf:fix-14
Closed

Wrap footnote definition in a paragraph#15
vhf wants to merge 1 commit intosyntax-tree:masterfrom
vhf:fix-14

Conversation

@vhf
Copy link
Copy Markdown
Contributor

@vhf vhf commented Sep 10, 2017

Here is a fix for #14

Please tell me what you think @wooorm !

@vhf vhf requested a review from wooorm September 10, 2017 21:04
@wooorm
Copy link
Copy Markdown
Member

wooorm commented Sep 26, 2017

Hey hey! Sorry about the long wait!

This looks good for text elements, but I’m wondering about a single strong, or other non-block element. What would happen there?

In the reverse program, we have something similar:

https://github.com/syntax-tree/hast-util-to-mdast/blob/1c494735fea6eec3bfedaf28afccd5fc82022a5e/lib/wrap.js#L26-L57

Do you think that’s useful to add here too?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Oct 22, 2017

I'm pretty sure that would be better, yep.

Since I'll need the list inline nodes in here, it's not ideal to maintain IMO.

A few ideas/solutions:

  1. creating a mdast package, only exporting nodes infos so that we could e.g. get a list of all inline nodes, or
  2. exporting this from remark, or
  3. attaching a type to each node besides the node type, e.g. {type: 'emphasis', element: 'inline'}

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Nov 6, 2017

Ahh, sorry, late!

I think a version of 1 makes the most sense, but in a separate utility? I think mdast should be used, if ever, for something like unist-builder but for mdast?

However, I think it makes sense to first inline it here, for now, before starting work on another utility?
Or, if it’s the same list as in hast-util-to-mdast, we could create a small utility and depend on it in both these projects?

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Nov 17, 2017

@vhf Ping! P.S. I know, I know, I’m late all the time 😒

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Nov 17, 2017

@wooorm I'm also constantly late, no worries. :)

It's the same list as hast-util-to-mdast. What would be a good package name?

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Nov 17, 2017

HTML doesn’t really have “block” or “inline”, as that’s related to CSS, but it does have “phrasing” content (warning: the HTML spec is huge), which contains <a>, <br>, <code>, <em>, <img>, <s>, <strong>, and Text.

So how about mdast-util-phrasing?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 3, 2017

@wooorm Here's my suggestion: https://github.com/syntax-tree/mdast-util-phrasing

If you would have preferred I created this repo outside of the org then moved it there upon your approval please say so! :)

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 3, 2017

No this is perfect! And what the organisation is for! Thank you!!! 🎉

So what are the next steps?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 3, 2017

Next step would be to bump the mdast-util-phrasing to 1.0.0 and publish it to NPM, then I'd change this PR to leverage it and also I could also PR hast-util-to-mdast to remove the array checked in here.

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 3, 2017

Sweet, sounds good! Could you add me on npm when you publish?

Would you like me to do something?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 3, 2017

I will. No need, I can handle this. I'll assign the PR reviews to you. :)

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 3, 2017

All done.

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 3, 2017

Now that I look at this again, is there actually a need for checking if there’s one node, and it’s phrasing? If footnote is always inline, why not always wrap it in a paragraph?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 4, 2017

I think you're right, footnote is always inline. (I deleted a comment I had just posted, I got confused with footnote and footnoreReference.)

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 4, 2017

Right?! That’s what I was thinking as well! Oh, I’m so sorry for leading you astray with my first comment #15 (comment)...

In that case, should we just wrap this in a <p>, always?

@vhf
Copy link
Copy Markdown
Contributor Author

vhf commented Dec 4, 2017

No big deal! Here it is again. 😄

@wooorm wooorm closed this in 0e8aa71 Dec 4, 2017
@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 4, 2017

Sweet!

@wooorm
Copy link
Copy Markdown
Member

wooorm commented Dec 4, 2017

Released, plus everything I can access that depends on it!

@vhf vhf deleted the fix-14 branch August 28, 2018 15:44
arobase-che added a commit to arobase-che/zmarkdown that referenced this pull request Jul 24, 2019
Since :
 - syntax-tree/mdast-util-to-hast#32
 - khttps://github.com/syntax-tree/mdast-util-to-hast/pull/15
@wooorm wooorm added 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧑 semver/major This is a change labels Aug 11, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have

Development

Successfully merging this pull request may close these issues.

2 participants