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

Add 2 new Markdown IT Functionality (include and container) #132

Merged
merged 15 commits into from
Dec 8, 2021

Conversation

L3houx
Copy link
Contributor

@L3houx L3houx commented Dec 7, 2021

  • Add documentation in README (about the 2 functionalities that I added)
  • Update package.json
  • Modify src/System/Tasks/ConversionRunner.ts to add 2 Markdown-It package (include and container)
  • Add a declare file to be able to compile the project du to the fact that markdown-it-include didn't had a @type

Thank You and Have a nice one!

@manuth manuth changed the base branch from main to dev December 8, 2021 07:33
Copy link
Owner

@manuth manuth left a comment

Choose a reason for hiding this comment

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

That's awesome! I've been looking for a convenient way to include other .md files and to apply CSS classes for long, tbh.

Before we can get started w/ merging this PR, please take note of the comments I've made and change your code appropriately.

In order to have your code checked correctly, you'll have to fix your code.

I assume the error that the CI is facing is caused by the package-lock.json file being generated using an npm version which is lower than v7.

Please perform following steps in order to fix it:

  • Revert changes made to package-lock.json: git checkout origin/dev -- package-lock.json
  • Install npm v7 (or higher - you might have to install a more recent version of nodejs in order to do so)
  • Re-run npm install

For a next time you might want to keep seperate features truly separate (i.e. create a separate PR for the CSS feature and a separate PR for the Include feature).

It's okay, though - we'll leave it as is for now.

Thanks a lot for the effort you put into this, I really appreciate it ✨

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/System/Tasks/ConversionRunner.ts Outdated Show resolved Hide resolved
src/System/Tasks/ConversionRunner.ts Outdated Show resolved Hide resolved
src/System/Tasks/ConversionRunner.ts Outdated Show resolved Hide resolved
src/System/Tasks/ConversionRunner.ts Show resolved Hide resolved
package-lock.json Show resolved Hide resolved
@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

Thank a lot by the way to take the time to describe them all like this, it's really appreciated. Once I did all you're recommandations, I update npm and nodejs to latests versions
node -v: v17.2.0
npm -v: 8.1.4

I had this error, Locally:

2021-12-08_12-13

Same as Build:
image

2021-12-08_12-15

I was able to fix it using hashFunction: "xxhash64" in the webpack config. I will wait your feedback and if needed, I will push back the webpack config

@manuth
Copy link
Owner

manuth commented Dec 8, 2021

@L3houx for some reason I don't know, the Drone CI build didn't start.
Anyways - there are still some linting issues. Could you please have a look at the output of npm run lint?

Thanks for taking care 😄

@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

npm run lint
image

I will check to fix these warnings

@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

Hi @manuth, I fixed the npm run lint

image

I will push my modifications soon.

@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

I have the same error as before, I'm waiting your call to update the webpack config to remove this error.

image

@manuth
Copy link
Owner

manuth commented Dec 8, 2021

Oh I'm sorry - I totally forgot about that.
I'll gladly accept your changes made to the webpack config, then 😄

@manuth
Copy link
Owner

manuth commented Dec 8, 2021

@L3houx running the tests failed.
I assume it's cause of this piece of code:

root: dirname(window.activeTextEditor.document.fileName),

However - I wanted to change how detecting the root works anyways.
If you don't mind I'll push my proposed changes directly to your PR.

@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

It's all good for me, I will wait your commit to use it in the markdown-it-include extension configuration

@manuth
Copy link
Owner

manuth commented Dec 8, 2021

@L3houx the CI tests will still fail
This is related to vscode applying IDs to headings by itself.

As this issue is unrelated to this very PR is ready to go.

Let me know if you are ready to have this PR merged or if you have something more to add.

@L3houx
Copy link
Contributor Author

L3houx commented Dec 8, 2021

It's all good for me :) You can press the merge button 😎

@manuth manuth merged commit 78d46ab into manuth:dev Dec 8, 2021
@manuth
Copy link
Owner

manuth commented Dec 8, 2021

Alright - did so! Thanks for your contribution, I really appreciate it 😄

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

Successfully merging this pull request may close these issues.

2 participants