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

Mjmlconfig #1206

Merged
merged 8 commits into from
Aug 16, 2018
Merged

Mjmlconfig #1206

merged 8 commits into from
Aug 16, 2018

Conversation

jskrzypek
Copy link
Contributor

@kmcb777 @iRyusa Following up on our discussion from #1131

I included a couple enhancements that seemed to make sense:

  • you can specify community components installed via npm by their standard requirable package names/paths (i.e. "mjml-chart" rather than "./node_modules/mjml-chart/lib/index.js"). I handle scenarios where this fails to resolve correctly.
  • if paths in the packages array resolve to a non-callable object whose properties are components (like mjml-accordion does), I iterate over the keys of the object and register the callable component classes instead

I'm going to create a simple test project and make sure this will work with mjml-charts and an additional custom component and then link the repo here so you guys can verify

@@ -83,14 +83,19 @@ export default async () => {
type: 'object',
describe: 'Option to pass to mjml-core',
},
cp: {
Copy link
Member

Choose a reason for hiding this comment

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

--config.configPath will be already defined, I think available options should only be listed in --config option instead

@iRyusa
Copy link
Member

iRyusa commented May 21, 2018

Hey !

That looks really great for me ! Just I would stick to current --config syntax over adding a new option

@jskrzypek
Copy link
Contributor Author

Here's my test repo: https://github.com/jskrzypek/mjml-config-test-project
I'll fix the config option thing now

@jskrzypek
Copy link
Contributor Author

jskrzypek commented May 23, 2018

@iRyusa if there are any other changes you'd like me to make on this pr, just let me know 😄

@iRyusa
Copy link
Member

iRyusa commented May 25, 2018

HI @jskrzypek

We just freeze our dev on 4.1 so it will be merged for a 4.1.1 👌

@kmcb777
Copy link
Collaborator

kmcb777 commented Aug 16, 2018

@jskrzypek nice PR, i'll merge it right now. I'll just change the option name configPath into mjmlConfigPath and the function registerCustomComponents into handleMjmlConfig because i also made a PR to make custom components work on the app and that's the names i used ;)

@kmcb777 kmcb777 merged commit e23898b into mjmlio:next Aug 16, 2018
@jskrzypek jskrzypek deleted the mjmlconfig branch September 16, 2018 15:14
@jskrzypek jskrzypek restored the mjmlconfig branch September 16, 2018 15:15
@jskrzypek jskrzypek deleted the mjmlconfig branch September 16, 2018 15:17
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.

3 participants