feat: add Gatsby logo to returned framework info #797
Conversation
ericapisani
added
the
type: feature
test/snapshots/main.js.md
Outdated
| @@ -33,6 +33,7 @@ Generated by [AVA](https://avajs.dev). | |||
| }, | |||
| env: {}, | |||
| id: 'sapper', | |||
| logos: undefined, | |||
There was a problem hiding this comment.
Question: Are we ok with leaving logos as undefined until they're all added?
remove react app since we are no longer using it
Shouldn't need it anymore with the react example site removed
also remove packages that were being used in the original test site, but are now not being used
there's an area within the netlify UI that uses this package and the earlier changes broke it
package.json
Outdated
| "last 2 Safari versions", | ||
| "last 2 iOS versions", | ||
| "last 2 ChromeAndroid versions" | ||
| ], |
There was a problem hiding this comment.
Given the demo app was removed from this project, it made sense to me that enforcement of this should likely from from the netlify FE project rather than here. If there's another reason why this should be added back in let me know
| @@ -33,6 +33,7 @@ Generated by [AVA](https://avajs.dev). | |||
| }, | |||
| env: {}, | |||
| id: 'sapper', | |||
| logo: undefined, | |||
There was a problem hiding this comment.
I'm curious why logo is being added here only for Sapper when the PR is adding only Gatsby logos at the moment.
There was a problem hiding this comment.
It's being added here because with the overall set of changes logo is being returned for all frameworks as part of the response body.
This particular snapshot is for a different test, and uses sapper instead of Gatsby, which is why this is appearing
| <body> | ||
| <a href="react">React Site</a> | ||
| </body> | ||
| <body></body> |
There was a problem hiding this comment.
[sand] Is the index.html file required since only assets are being served now?
| const originalLogo = contents.logo | ||
| if (originalLogo) { | ||
| for (const [theme, urlPath] of Object.entries(originalLogo)) { | ||
| updatedContents.logo[theme] = (process.env.DEPLOY_PRIME_URL || 'https://framework-info.netlify.app') + urlPath |
There was a problem hiding this comment.
[dust] Prefer destructuring.
| const originalLogo = contents.logo | |
| if (originalLogo) { | |
| for (const [theme, urlPath] of Object.entries(originalLogo)) { | |
| updatedContents.logo[theme] = (process.env.DEPLOY_PRIME_URL || 'https://framework-info.netlify.app') + urlPath | |
| const{ logo } = contents | |
| if (logo) { | |
| for (const [theme, urlPath] of Object.entries(logo)) { | |
| updatedContents.logo[theme] = (process.env.DEPLOY_PRIME_URL || 'https://framework-info.netlify.app') + urlPath |
There was a problem hiding this comment.
[dust] Similar comment to @nickytonline's about destructuring - I prefer template strings rather than string appends. Perhaps we codify stuff like this in our ESLint config if we don't have rules for this already.
Summary
Related issue
I removed the original test site and replaced it with just the
assetsfolder which over time will contain all the logos for the various frameworks.The cypress tests were also removed since they were testing that demo site, and various dependencies for the demo site were also removed (e.g: babel, react, webpack).
Test plan
build-infoto confirm that thelogoproperty was being returned as expectedframework-infolinked, logging in and going to create a new site, selected a nextJS project, confirmed that the default build commands were populated and a message indicating that a NextJS project was detected appearedA picture of a cute animal (not mandatory, but encouraged)