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

Added list of errors on sign up request in the docs #116

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

silacs
Copy link

@silacs silacs commented Feb 5, 2025

No description provided.

@silacs
Copy link
Author

silacs commented Feb 6, 2025

@CondensedMilk7 turned it into a table, I think that commit automatically got added to this pull request too right?

@CondensedMilk7
Copy link
Member

@KostaD02 I get the same errors as @silacs, there's an issue with node-gyp and it might be the case the package-lock got messed up. Maybe try reviewing and testing on your side.

@KostaD02
Copy link
Member

KostaD02 commented Feb 8, 2025

@silacs is it ready for review?

@silacs
Copy link
Author

silacs commented Feb 8, 2025

@KostaD02 yeah, the thing I wanted (possible errors on signup) is ready.
might add errors on other things too, and also interfaces of things, but I don't have time for that for now.

| `"errors.invalid_avatar"` | When avatar url is not a url |
| `"errors.invalid_gender"` | When gender is not `'MALE'`, `'FEMALE'` or `'OTHER'` |
| `"errors.email_in_use"` | When email is already registered |
<!-- | `"errors.teapot"` | When brewing coffee is requested | -->
Copy link
Member

Choose a reason for hiding this comment

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

Whats the point? would be better without comment.
Preferably to not keep unnecessary code

Copy link
Author

@silacs silacs Feb 9, 2025

Choose a reason for hiding this comment

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

forgot about that lol, you can remove it before accepting the pr right? I remember I checked some toggle allowing modifications or something like that

i'm not home right now and cant change it myself

should be home by the end of the day though, if you haven't changed it by then, then i will

Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you update it

@@ -88,6 +88,30 @@ curl -X 'POST' \
Email verification may also be required.
:::

::: info NOTE
Copy link
Member

Choose a reason for hiding this comment

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

Imo it would be better without alert

Copy link
Author

Choose a reason for hiding this comment

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

what alert? you mean :::?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the table inside the note could be a little unreadable.

For example this is a note:

image

Copy link
Author

@silacs silacs Feb 9, 2025

Choose a reason for hiding this comment

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

yeah I know, but I built the docs and tested it and it looked fine imo, will show you a screenshot im going home soon

Copy link
Member

Choose a reason for hiding this comment

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

Could you screenshot light and dark mode?

Copy link
Author

@silacs silacs Feb 9, 2025

Choose a reason for hiding this comment

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

image
image

Without :::
image
image

Yeah now looking at it without ::: I think it looks better, but its fine with ::: too, what do you think which one is better? I already fixed the errors documentation issue and removed the code comment, i'll commit when you decide on this

Copy link
Author

Choose a reason for hiding this comment

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

actually, would this be better in errors.md? just making a section for signup (and also the others when I add them)

Copy link
Member

Choose a reason for hiding this comment

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

@silacs sorry for the late response. Let's keep it without an alert; a single table looks better. It would be great if each endpoint had its own error mapping.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, im thinking of making it like how the shop is in the sidebar
so
errors
ⳑ Auth
ⳑ Products
ⳑ Cart
and then each category will have errors for each endpoints

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Could work but each time user has to search it, would be better if it will be with endpoint description

@KostaD02 KostaD02 added the documentation Improvements or additions to documentation label Feb 11, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants