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

Code snippets for solid #21207

Merged
merged 41 commits into from
Apr 27, 2023

Conversation

webblocksapp
Copy link
Contributor

@webblocksapp webblocksapp commented Feb 22, 2023

What I did

  • After mergin this PR, Frontpage PR can be merged.
  • Added documentation code snippets for SolidJS, there are still pending some code snippets to add for:
    -- Mocking GraphQL queries with MSW addon
    -- Setup the testing addon for your framework
  • Added the file docs/sync.js for synchronizing docs with frontpage repo due to link-monorepo-docs command is not working.
  • Updated /docs/contribute/new-snippets.md with new steps for updating code snippets in frontpage, due to link-monorepo-docs is unusable.

How to test

  • Follow /docs/contribute/new-snippets.md guide to setup frontpage and the monorepo docs synchronization.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

docs/sync.js Outdated
@@ -0,0 +1,74 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

@shilman — Is there any issue with including a script file inside the docs directory? If not, I'm thinking we could merge this for now, then move it to the root (possibly as a task command?) later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can do this improvement by converting the sync.js file into a storybook task.

docs/sync.js Outdated
const run = async () => {
let frontpageDocsPath = '/src/content/docs'

const frontpageAbsPath = await askQuestion('Provide the frontpage project absolute path:')
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this with a relative path ../../frontpage and it works fine. Suggest asking for a relative path, as the absolute path can be unwieldy and is seldom known off-hand.

Copy link
Contributor Author

@webblocksapp webblocksapp Mar 6, 2023

Choose a reason for hiding this comment

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

Perfect, I will update the command message for asking for a relative path. Also docs.

Then change your current directory path to the docs folder. Synchronize the documentation file changes with the Storybook website by running the script `sync.js`.

```shell
cd docs && node sync.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth pointing out that this is an ongoing process, so they should run it in a separate terminal window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be by converting the sync.js into a task will simplify more the docs.

* to learn how to generate automatic titles
*/
title: 'ButtonGroup',

Copy link
Contributor

@kylegach kylegach Feb 28, 2023

Choose a reason for hiding this comment

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

I'm surprised that the pretty-docs check (which runs prettier linting on docs snippets) passed with empty lines like this. Mind removing them (I see them throughout these snippets)?

Copy link
Contributor

@jonniebigodes jonniebigodes Feb 28, 2023

Choose a reason for hiding this comment

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

@kylegach, it's from the pr that Kasper pushed for the initial React snippets. I've already updated them on my end and they should fixed shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@webblocksapp can you follow up on Kyle's feedback and remove the empty lines in the examples as the remainder snippets were already fixed to factor in this issue?

@@ -0,0 +1,26 @@
```js
Copy link
Contributor

Choose a reason for hiding this comment

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

For snippets that contain JSX, we should use jsx for the language.

(I realize this is also incorrect for our React—and likely other–snippets. ☹️)


export const ExampleStory = {
render: (args) => {
const [someFunctionResult, setSomeFunctionResult] = createSignal();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with SolidJS. Is there someone from the community that could review the SolidJS-specific parts of this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do the review of the code snippets on a separate sandbox project, so I can attach a link to this PR with the sample repo so you can test from those implementations. Let me know if this works for you.

Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Wow! 🙌 What a huge effort. Thank you, @webblocksapp!

I left a few comments, but the main thought is that I'm not actually qualified to review the SolidJS-specific aspects of this work. I'm OK to ship this as-is and revise from there, but I advise finding another community member with SolidJS experience to review.

@webblocksapp
Copy link
Contributor Author

webblocksapp commented Mar 25, 2023

Hi @jonniebigodes and @kylegach,

I hope this message finds you well. I wanted to bring your attention to some recent updates that I just pushed to the repository. Most of the code snippets have been thoroughly tested in a sandbox repository using Storybook, giving us greater confidence that these examples are working as intended. If you'd like to check it out, the sandbox repository can be found here.

In addition, we've added a new command at yarn task for synchronizing docs, which will make it easier for developers to get involved and contribute to the frontpage docs. You can find the documentation for this command at docs/contribute/new-snippets.md.

I suggest that we consider this approach as a first iteration for the SolidJS documentation and seek feedback from the community to gather insights and make necessary improvements in the future.

After this PR revision and merge, you can move forward with the Frontpage PR.

Thank you for your time and consideration.
Good weekend!

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@webblocksapp Sorry for the wait on this, but as you may have heard, Storybook 7 is out and now I've had some time to review this pull request in more detail and with that I left some small items for you to address so that we can merge this and the companion pull request and start documenting the SolidJS framework.

Let me know once you've addressed this and we'll go from there.

Hope you have a great weekend.

Stay safe

* to learn how to generate automatic titles
*/
title: 'ButtonGroup',

Copy link
Contributor

Choose a reason for hiding this comment

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

@webblocksapp can you follow up on Kyle's feedback and remove the empty lines in the examples as the remainder snippets were already fixed to factor in this issue?

docs/snippets/solid/button-story-auto-docs.ts.mdx Outdated Show resolved Hide resolved
docs/snippets/solid/button-story-decorator.ts.mdx Outdated Show resolved Hide resolved
docs/snippets/solid/mdx-canvas-multiple-stories.mdx.mdx Outdated Show resolved Hide resolved
docs/writing-stories/introduction.md Outdated Show resolved Hide resolved
@webblocksapp
Copy link
Contributor Author

Hi @jonniebigodes, I wanted to let you know that I've addressed the feedback from the code review. Feel free to let me know if you have some other findings. Thanks!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Hi @webblocksapp !! I tried out the sync-docs task. It's a really cool idea.

I had a couple minor comments about the experience:

  1. I ran it on an unsynced frontpage project. It errored because the src/content/docs directory didn't exist yet. If the src/content directory exists but there is no docs directory, it should create a docs directory for you.
  2. I manually created the docs directory. However it was empty. Then every time I updated the monorepo it copied the changes over to the new docs directory. However, the rest of the content was not copied over, so the workflow wasn't really working. If you are creating the the docs directory per the previous point, maybe the task should copy over the entire contents on initialization?

Another thought is... would it be simpler to just use symbolic links instead & update the documentation accordingly, rather than creating a new task?

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@webblocksapp, thanks for addressing the feedback so promptly. Appreciate it 🙏 ! From my end, I just need for you to look into a small item regarding the ts-4-9 variants of the snippets, you already addressed it in some, but I was wondering if you could take another pass at them and adjust accordingly. Also, when you have a chance, can you take a look at the feedback from @shilman so that we can get this one merged and update the documentation?

Looking forward to hearing from you.

Hope you have a great week.

docs/snippets/solid/button-group-story.ts-4-9.mdx Outdated Show resolved Hide resolved
@webblocksapp
Copy link
Contributor Author

webblocksapp commented Apr 24, 2023

Hi @shilman, thxs for your feedback!

Another thought is... would it be simpler to just use symbolic links instead & update the documentation accordingly, rather than creating a new task?

Currently storybook frontpage repo has a command for creating a symbolic link, but this solution is not workable, so this command will be removed with the associated frontpage PR. I had this conversation with @kylegach at wg-solid Storybook Discord channel, the issue is that gatsby is not able to find node_modules of the symlinked packages. So for this reason I started this initiative of building a command for synching docs.

Related to points 1 and 2, you are right. The synchronization docs command only is working for individual files. It's not doing an initial copy of the docs folder at frontpage repo before starting the watcher for checking file changes. But I think it would be better to complete the synchronization flow. so the final idea could be that every time the sync docs command is executed, the frontpage docs folder would be recreated from scratch, and then start the file watcher for updating changes.

Please, let me know if this final solution sounds good, so I can add it to the command.

@shilman
Copy link
Member

shilman commented Apr 24, 2023

@webblocksapp your solution sounds great to me. thanks for taking care of this!! 🙏

@webblocksapp
Copy link
Contributor Author

Hello @jonniebigodes and @shilman, I wanted to let you know that I've just pushed new updates to the code based on your feedback. Please take a look and let me know if there's anything else that needs to be refined. Thank you!

@jonniebigodes
Copy link
Contributor

@webblocksapp I've just checked, and all is good on my end. Thanks once again for addressing the feedback so promptly. Appreciate it 🙏 ! I'll check with the team, and if there's nothing blocking it, we can finally merge this one and the associated pull request in the frontpage repository to get the documentation updated and improve the UX for contributions.

Hope you have a fantastic day.

Stay safe

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback @webblocksapp !! Amazing job on this PR 👏

@jonniebigodes jonniebigodes added patch:yes Bugfix & documentation PR that need to be picked to main branch and removed ci: do not merge labels Apr 27, 2023
@jonniebigodes
Copy link
Contributor

@webblocksapp going to merge this. Once again, thanks for the amazing job with the contribution 🎉

@jonniebigodes jonniebigodes merged commit 330c365 into storybookjs:next Apr 27, 2023
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 3, 2023
shilman pushed a commit that referenced this pull request May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants