-
Notifications
You must be signed in to change notification settings - Fork 343
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
feat(command-init): recommend build plugins #1836
Conversation
This is cool, @erezrokah! Is it possible to add an explanatory line before the question prompt? |
Great idea! Of course we can |
🎉 @rstavchansky Can you work with @erezrokah to develop an appropriate message? Since this functionality will be used for multiple frameworks/plugins, I'm guessing this will need to be a more general message, like, "Based on the technology used in your site, we recommend the following plugin(s):" |
After some discussion in Slack to understand the mode of detection, I'm making this recommendation: Single recommended plugin flow
Multiple recommended plugins flow
One thing to consider - Is it possible to add a link to the plugin's readme so people can see what they're installing? (I supposed you’d have to get that link from the plugins.json file, though, unless you could predict it programmatically.) A note on capitalization from the Style Guide: Build Plugins is capitalized since it's a formal feature name. plugins is not. |
Sure we can add the link |
UpdateThis is pending a decision if to use plugin package names or plugins names. |
7c311ab
to
81ef433
Compare
81ef433
to
69b7d81
Compare
If the human-readable names are available, I think those are best. |
238ba5f
to
b80e574
Compare
28dee85
to
8f021c6
Compare
} | ||
|
||
test('netlify init existing site', async (t) => { | ||
const initQuestions = [ |
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.
Figured it's time for us to be able to test the CLI input prompts. If you think of a better way to do this please let me know.
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.
This is great! 🎉
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.
👍 🚀
const getFrameworkInfo = async ({ siteRoot, nodeVersion }) => { | ||
const frameworks = await listFrameworks({ projectDir: siteRoot, nodeVersion }) | ||
if (frameworks.length !== 0) { | ||
const [ |
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.
Are we only considering the first element of the array on purpose (i.e. the framework-info
lib is only returning one framework for the time being)?
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.
It might detect a few frameworks - the first one has higher priority based on the order in https://github.com/netlify/framework-info/blob/32c58bb8de2f86874a61bdc7abab059c4c638f6c/src/frameworks/main.js#L4.
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 wouldn't mind having a comment here just highlighting that (we pick the first as its the highest priority). Not critical though, happy to move forward if you feel like it doesn't make sense @erezrokah 👍
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.
That makes sense. I'll add it
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.
Done in e1a410e
} | ||
|
||
test('netlify init existing site', async (t) => { | ||
const initQuestions = [ |
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.
This is great! 🎉
8f021c6
to
e1a410e
Compare
FYI, this is pending the release of netlify/next-runtime#94 (it was merged but not released). cc @lindsaylevine |
Just a suggestion, and please show me the door if you think this is crazy or stupid, but wondering if some iconography (utilizing emojis) might help to grok this (since the message is a bit longer than the other one-liner prompts), as well kind of indent the text under the first line. So... something like this, maybe? Seems like this is a {detected framework} site. Again, totally a non-blocking suggestion! Feel free to ignore. |
released to npm, pending this PR to plugins netlify/plugins#222 |
fa06c9b
to
d69403b
Compare
- Summary
Fixes #1832 pending netlify/framework-info#123 and netlify/next-runtime#93
- Test plan
Going to do some more testing once the blocking issues are completed
- Description for the changelog
Recommend plugins during
ntl init
command and let the user choose if to install them- A picture of a cute animal (not mandatory but encouraged)
TODO
framework-info