-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
fix: allow also " inside of an embed #1598
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: allow also " inside of an embed #1598
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/BJiGEnyE33rV1Pzk3UTtre7AHZqb |
All reactions
Sorry, something went wrong.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 633f241:
|
All reactions
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this?
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sy-records
As we all know, this func getAndRemoveConfig
has been used for much places.
It seems we need separate it for different functions in case of unpredicted issues.
WDTY?
Sorry, something went wrong.
All reactions
Yes, you're right. |
All reactions
Sorry, something went wrong.
@Koooooo-7 What do you mean? |
All reactions
Sorry, something went wrong.
This func r working in multi place to get configs. such as titles , sidebar headings. |
All reactions
Sorry, something went wrong.
@Koooooo-7 I see what you're saying, although it seems very unlikely. My guess would be that there is no one relying on he non-working double quotes. It seems multiple people have hit this issue, and we closed the issues. It was in fact working before, then it broke: |
All reactions
Sorry, something went wrong.
sure, will try that.
I dont think so. I would also like to use the double ones inside of headlines, or other stuff. Everywhere I can use this style. And another question, do you prefere an clean commit history, so should I rebase merge upstream changes? |
All reactions
Sorry, something went wrong.
@luwol03 of cuz we can support this feat for those configs. instead, how about those titles/other stuff without configs but with " ? |
All reactions
Sorry, something went wrong.
Then we should improve the function, so that it only replaces the quotes, if they are followed by a |
All reactions
Sorry, something went wrong.
agreed. |
All reactions
Sorry, something went wrong.
Is this normal, that your e2e tests failed? |
All reactions
Sorry, something went wrong.
It seems not this changes broken, it's okay. |
All reactions
Sorry, something went wrong.
@Koooooo-7 can you point them out for us? ESIT: Oh NVM, i see you checked already and think it is fine. |
All reactions
Sorry, something went wrong.
Sometimes it fails for some reason despite that it shouldn't. Needs a fix. |
All reactions
Sorry, something went wrong.
Before merge, I will try that:
|
All reactions
Sorry, something went wrong.
Is anyone using it without the |
All reactions
Sorry, something went wrong.
Here is an example without the colon: #1587 (comment)
|
All reactions
Sorry, something went wrong.
str: `[filename](_media/example.md ":include")`, | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what the behavior should actually be. Seems funky that it leaves things behind. Seems like it should handle everything.
Sorry, something went wrong.
All reactions
@jhildenbiddle One test or other keeps randomly failing. |
All reactions
Sorry, something went wrong.
Investigating the failures. Let's not merge until we know it isn't the change from here. Seems unlikely that it is. |
All reactions
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'm not requesting any changes, just blocking from merging to ensure the test is just flaky (that's what it seems like).
Sorry, something went wrong.
All reactions
I re-ran the test like 6 times already, and one (at random) still keeps failing. |
All reactions
Sorry, something went wrong.
I don't think we should do that, it could break this: Thanks @luwol03! Merging, since tests pass except one at random, which seems unrelated. |
All reactions
Sorry, something went wrong.
Oh, A few seconds ago, I pushed my changes, before I relized, that you already merged this changes 😂 Can you please take a look at my new commit? @trusktr Should I open a new PR for that? |
All reactions
Sorry, something went wrong.
@luwol03 You can open a new PR |
All reactions
Sorry, something went wrong.
|
All reactions
Sorry, something went wrong.
sy-records
trusktr
Koooooo-7
Successfully merging this pull request may close these issues.
embed quotes bug ":include" in double quotes doesn't recognized ':include' not working in Docsify 4.12.1 Embed files does not work
Summary
I fixed a bug where
":include"
is not handled like':include'
. This is not working because the type of quotes are not supported. The default prettier configuration changes the''
to""
. And if you have auto format toggled on, you're not be able to save the file with the right quotes.What kind of change does this PR introduce?
I added added the
"
also the the replace regexFor any code change,
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
Related issue, if any:
closes #1597
closes #1587
closes #1555
closes #1005
Tested in the following browsers: