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 PropShop and add descriptions to interface #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PaulieScanlon
Copy link

Hi @molebox hope you don't mind but i've added my new plugin gatsby-plugin-prop-shop to your theme.

Don't feel like you need to merge this but it was very helpful for me to be able to test this plugin in a TypeScript project. There were some differences with how react-docgen grabs the data from the interface prop definitions... i've moved the descriptions to the interface too otherwise my plugin can't find them to display in the PropTable.

  • If you run yarn to update all the dependancies
  • Then run yarn workspace demo dev
  • Then visit http://localhost:8000/prop-shop/ you should see all prop definitions and descriptions for files found in your themes src/components/ directory... it's only the SEO component as i'm sure you know...

I'd be keen to hear what you think about the plugin.

Thanks in advance!

@molebox
Copy link
Owner

molebox commented Feb 15, 2020

Hey Paulie, looks good! Im not sure if its the contrats on my screen but your prop-shop page is kinda hard to read with the very light text and very light blues. I like how it looks design wise though, and it looks like a really handy plugin. I dont know what value it would add to an SEO component. I could merge this anyway as you have fixed up the interface comments.

I had a change locally which i havent pushed (forgot about it) where i moved the twitter input from siteMetadata to component input, so i removed that as you have done that in this PR.

If you dont have anything further to add, or comments then id be happy to merge this. One thing would be the static version of your plugin. im not sure how it works with tags. Does the carat mean that it will install the latest upto the first patch?

@PaulieScanlon
Copy link
Author

PaulieScanlon commented Feb 15, 2020

Great point about the colours, I’m developing in the sun and I’ve had to move to the shade to see it properly. I’ll give the design another blast.

No worries if you don’t wanna merge it in, as you say you only have the one component and PropShop is really for larger component library projects but your theme was a good alpha test because I hadn’t previously tested it with TypeScript

Hmm with regard to the carat, I’m not sure. I think that won’t apply because I’m using an -alpha suffix, It might make sense to wait until I have a stable release before you merge... if you find it at all useful of course.

This PR was a kinda test and to gather some feedback which I’m grateful for! so thanks!

Feel free to close this with a “won’t fix” If you like... I won’t be offended I promise 😀

@molebox
Copy link
Owner

molebox commented Feb 15, 2020

I think I'll cherry pick your interface changes and close. As you say, I think your plugin is best suited to actual libs with components.

Glad i could provide some feedback though, you're on fire with these plug-ins lately!

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.

None yet

2 participants