-
Notifications
You must be signed in to change notification settings - Fork 16
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: adds auto generate showcases #1296
Conversation
✅ Deploy Preview for starter-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Look good to me.
I Leave it for others to review
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 PR will likely be best held until the new design PR is approved and merged - there's a reasonable bit of crossover between the two and having the showcases doesn't make sense without that new design.
There will be a few places we'll need to adjust once the design PR (#1291 ) is merged:
- in
lib/parseKits
, I think we'll be able to adjust that to pick the showcases from the package.json instead of having the variable there - the showcase variable will need to be removed from the config file
- in that showcase variable we're also setting the icon - that doesn't seem to be handled in the current PR, so will need to be solved again
I've left a few other questions and suggestions for this PR, most fairly minor.
We should also update the root level README to indicate that new kits should include the "showcase" array, and what that should include / look like.
I also wonder if we need to update the packages/website predev
and prebuild
scripts to also generate the showcases? We'll likely need that to ensure things are kept up to date as well.
Yeah, I was initially planning on #1291 being merged before this was completed, but that had a few rounds of changes and is still ongoing. I was considering pushing this into #1291, but I also didn't want to hold that up with any feedback on this and cause a massive delay on it like in previous PRs/tickets. With that route chosen, I ended up removing the hook-up functionality I had originally implemented for instance
I was going to create a new ticket that would focus on combining those above-needed changes with the new updates from #1291. I'm good with either outcome and I will be pushing a new commit that should address a majority of the feedback you've given. 😄 |
… prebuild, and root lvl README
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 solid to me! The only tiny nitpick I'd make is - the array in each kit's package.json file, can we name it "showcases" instead of "showcase" (make it plural)? Since the idea is that we'll eventually have multiple showcases? Otherwise, this feels good!
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Type of change
Summary of change
Checklist