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

docs: add string() #220

Merged
merged 25 commits into from
Dec 5, 2023

Conversation

kazizi55
Copy link
Contributor

@kazizi55 kazizi55 commented Oct 21, 2023

Implements #209 (comment)

@netlify
Copy link

netlify bot commented Oct 21, 2023

Deploy Preview for valibot ready!

Name Link
🔨 Latest commit 774f0ab
🔍 Latest deploy log https://app.netlify.com/sites/valibot/deploys/656f628c6616e0000820f045
😎 Deploy Preview https://deploy-preview-220--valibot.netlify.app/404
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kazizi55 kazizi55 marked this pull request as ready for review October 21, 2023 06:58
@fabian-hiller fabian-hiller self-assigned this Oct 21, 2023
@fabian-hiller fabian-hiller added the documentation Improvements or additions to documentation label Oct 21, 2023
@fabian-hiller
Copy link
Owner

Thank you very much! I'll take a closer look at the PR in the next few days and give you feedback.

@fabian-hiller fabian-hiller added the priority This has priority label Oct 21, 2023
Copy link
Owner

@fabian-hiller fabian-hiller left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution! 🧩

website/src/routes/api/(schemas)/string/index.mdx Outdated Show resolved Hide resolved
website/src/routes/api/(schemas)/string/index.mdx Outdated Show resolved Hide resolved
website/src/routes/api/(schemas)/string/index.mdx Outdated Show resolved Hide resolved
## Parameter

- `errorMessage` <Property type='string'/> = 'Invalid type'
- `pipe` <Property type={{ type: 'custom', name: 'Pipe<string>' }}/> = []
Copy link
Owner

@fabian-hiller fabian-hiller Oct 23, 2023

Choose a reason for hiding this comment

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

The syntax name: 'Pipe<string>' is not quite correct:

<Property
  type={[
    {
      type: 'custom',
      name: 'Pipe',
      href: '../Pipe',
      generics: ['string'],
    },
    'undefined',
  ]}
/>;

Copy link
Owner

Choose a reason for hiding this comment

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

To not mess up the mdx file, you can also define complex properties at the end of the file like I did in Modular Forms: https://github.com/fabian-hiller/modular-forms/blob/main/website/src/routes/(layout)/[framework]/api/getValues.mdx

Copy link
Owner

Choose a reason for hiding this comment

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

I would be interested to know how other documentation handles this internally. Maybe we can use that to improve our approach with <Property />.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented!
a224502

I would be interested to know how other documentation handles this internally. Maybe we can use that to improve our approach with .

I'll look this up in the coming days 🙏

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 would be interested to know how other documentation handles this internally.

I've searched 3 library documents, and found that there are only simple approaches to handle type and property information:

So as far as I've searched, I guess it might be good to use without big changes.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for the research!

website/src/routes/api/(schemas)/string/index.mdx Outdated Show resolved Hide resolved
website/src/routes/api/(schemas)/string/index.mdx Outdated Show resolved Hide resolved
@fabian-hiller
Copy link
Owner

You are great! Thanks a lot. I will try to review the changes in the next few days.

One idea I had was to list the authors of the documentation at the bottom of each page, like in Qwik. What do you think about that? Here is an example: https://qwik.builder.io/docs/

@fabian-hiller
Copy link
Owner

@kazizi55 please pull my latest changes, start the website in your development environment and let me know what you think about them.

@kazizi55
Copy link
Contributor Author

kazizi55 commented Nov 5, 2023

Thanks to all the contributors who have helped make this documentation better!

Sounds great!
This would motivate contributors to improve docs more 😄

Can I add this section in another PR?

@kazizi55
Copy link
Contributor Author

@fabian-hiller

Currently, generics in types cannot be fully mapped with . I noticed this with StringSchema for example. We'll have to look at this again.

Fixed!
2c4c0b9

Now, generics with default parameter can be displayed!
https://deploy-preview-220--valibot.netlify.app/api/StringSchema/

@@ -54,7 +54,9 @@ type SingleTypeOrValue =
| {
type: 'custom';
name: string;
generics?: TypeOrValue[];
generics?:
| { withDefault: true; name: string; type: TypeOrValue }
Copy link
Owner

Choose a reason for hiding this comment

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

Can we rename that to default so that it is similar to the default prop of the component?

Copy link
Owner

Choose a reason for hiding this comment

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

My idea would be to change it to default?: TypeOrValue.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe your approach is the right one. I will probably take a closer look tomorrow.

Copy link
Contributor

@wout-junius wout-junius left a comment

Choose a reason for hiding this comment

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

LGTM

@fabian-hiller
Copy link
Owner

Thanks a lot. I plan to review the new changes in the next days.

@fabian-hiller
Copy link
Owner

@kazizi55 can you review my changes? I think that we are slowly approaching our destination. 🏁

@kazizi55
Copy link
Contributor Author

@fabian-hiller
LGTM! Thank you for your changes 😄
What do you think about the next actions to complete the API document?
Which pages should I write next?

I think it's better to create a new issue which includes to-do list, so that we can check which document is completed or not. 😄

@fabian-hiller
Copy link
Owner

Are you happy with the current version? Would you change anything? I will take another look at everything in the next few days and also check how other API references present there information to give feedback. But we are on the right track and I think we will reach a final version soon.

@fabian-hiller
Copy link
Owner

I have further improved the string API reference and updated the references of all types. I'll take another look at it tomorrow. I think we can merge the changes shortly and then add the other API references one by one. 🚀

@fabian-hiller
Copy link
Owner

@kazizi55 thank you again for your contribution! I will merge this PR and post the draft on Twitter to get more feedback before we add the other references.

@fabian-hiller fabian-hiller merged commit 95b4445 into fabian-hiller:main Dec 5, 2023
10 checks passed
@fabian-hiller
Copy link
Owner

@kazizi55
Copy link
Contributor Author

kazizi55 commented Dec 7, 2023

@fabian-hiller
Sorry for my late reply 🙏

I will merge this PR and post the draft on Twitter to get more feedback before we add the other references.

Thank you!
Have you received any feedback so far?

@fabian-hiller
Copy link
Owner

Unfortunately, no. We probably only get feedback when people use the API reference and don't understand or miss something. I will create a issue soon where we can coordinate the API reference extension. 🧩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation priority This has priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants