Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Wrap footnote definition in a paragraph #15

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

Closed
vhf wants to merge 1 commit into syntax-tree:master from vhf:fix-14
Closed

Conversation

@vhf
Copy link
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
Copy link
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?

Copy link
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'}

Copy link
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?

Copy link
Member

wooorm commented Nov 17, 2017
edited
Loading

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

Copy link
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 reacted with thumbs up emoji

Copy link
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?

Copy link
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 reacted with hooray emoji

Copy link
Member

wooorm commented Dec 3, 2017

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

So what are the next steps?

Copy link
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.

Copy link
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?

Copy link
Contributor Author

vhf commented Dec 3, 2017

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

wooorm reacted with heart emoji

Copy link
Contributor Author

vhf commented Dec 3, 2017
edited
Loading

All done.

Copy link
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?

Copy link
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.)

Copy link
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?

Copy link
Contributor Author

vhf commented Dec 4, 2017

No big deal! Here it is again. 😄

Copy link
Member

wooorm commented Dec 4, 2017

Sweet!

Copy link
Member

wooorm commented Dec 4, 2017

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

@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

Reviewers

@wooorm wooorm wooorm approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /