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

Update and fix custom condition documentation #6424

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Conversation

benstigsen
Copy link
Contributor

#6361

Description

After having tried to create a custom condition for my extension, I ran into several issues following the documentation.

There were minor errors such as true being written as strue. But there were also some major issues, like things being imported which weren't used, extending the wrong classes and in general unclear ways to do stuff.

I ran into an issue requiring many hours of debugging, reading documentation and trial-and-error, where the custom condition was extending the wrong class, leading to something like args.host being undefined, resulting in the entire extension never being permitted, never reaching the timeout.

It also wasn't being shown how to make use of match, even though a condition config type was created to allow for this.

Type of suggestion

  • Typo/grammar fix
  • Updated outdated content
  • New content
  • Updates related to a new version
  • Other

Product & version (if relevant)

Umbraco 14

Copy link
Contributor

@sofietoft sofietoft left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for remaking the PR @benstigsen ! 😄 👏

I've gone through it, in an attempt to make it flow a little better with the next between the code snippets.
Please consider my suggestions when you have a minute 🙌

benstigsen and others added 3 commits September 17, 2024 12:20
…condition.md

Co-authored-by: sofietoft <stk@umbraco.com>
…condition.md

Co-authored-by: sofietoft <stk@umbraco.com>
…condition.md

Co-authored-by: sofietoft <stk@umbraco.com>
@benstigsen
Copy link
Contributor Author

@sofietoft sure thing! I've committed your suggested changes 😄

@sofietoft
Copy link
Contributor

Thanks for considering the suggestions @benstigsen ! 💪

Looks like we're finally ready to get this published.
I'll merge the PR 🎉

@sofietoft sofietoft merged commit 149d2a9 into umbraco:main Sep 18, 2024
4 checks passed
@benstigsen
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants