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

fix(code-block): meta property #2067

Merged
merged 9 commits into from May 22, 2023
Merged

fix(code-block): meta property #2067

merged 9 commits into from May 22, 2023

Conversation

farnabaz
Copy link
Member

@farnabaz farnabaz commented May 16, 2023

πŸ”— Linked issue

resolves #2059

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@netlify
Copy link

netlify bot commented May 16, 2023

βœ… Deploy Preview for nuxt-content canceled.

Name Link
πŸ”¨ Latest commit 04aa05f
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt-content/deploys/646b8892cc246c0008a20897

Comment on lines 21 to 23
const highlightTokens = lang.match(/{([^}]+)}/)
const filenameTokens = lang.match(/\[(.+)\]/)
const meta = lang.replace(/^\w+\s*(\[[^\]]+\])?\s*(\{[^}]+\})?\s*/g, '')
Copy link
Contributor

@nobkd nobkd May 16, 2023

Choose a reason for hiding this comment

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

Previous comment:

A problem is, that the meta regex has a fixed order for lang […] {…} meta
It is not replaced if we write lang {…} […] meta, even though L21-22 allow different ordering.

Also, if we want to use {…} or […] in the meta block, but not provide highlights or a filename, it will be matched (by L21-22).
Maybe, the user could provide an empty [] or {} block before the meta to prevent it from reading those parts in the meta.
Something like: lang [] meta=['not', 'a', 'filename'] would be possible then


In my opinion, we should also trim the lang property before checking if it is empty (adding it in L11)

lang = lang.trim()

Also return undefined (or an empty string '') for the meta if lang is empty (adding in L17)

{
  // ...
  meta: undefined
}

Copy link
Contributor

@nobkd nobkd left a comment

Choose a reason for hiding this comment

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

Looks good to me.

But I don't know if it's good to return empty strings on empty filename, or if we should return undefined instead... πŸ€”

What do you think?

Copy link
Contributor

@nobkd nobkd left a comment

Choose a reason for hiding this comment

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

This would be my change, if we don't want to return empty strings.

src/runtime/markdown-parser/handler/utils.ts Outdated Show resolved Hide resolved
test/features/parser-markdown.ts Outdated Show resolved Hide resolved
farnabaz and others added 2 commits May 22, 2023 17:13
Co-authored-by: nobkd <44443899+nobkd@users.noreply.github.com>
@farnabaz farnabaz merged commit e19113d into main May 22, 2023
7 checks passed
@farnabaz farnabaz deleted the fix/code-meta branch May 22, 2023 15:32
@farnabaz farnabaz mentioned this pull request Jun 13, 2023
@nobkd nobkd mentioned this pull request Jul 13, 2023
7 tasks
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.

ProseCode doesn't receive the right prop value for meta
2 participants