-
Notifications
You must be signed in to change notification settings - Fork 58
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
FED-178 Add Documentation for uiJsComponent
#769
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final pass, this is looking super close! Awesome stuff
Applied a hold because I'm going to pass this along to at least one consumer (after we're done reviewing it) to make sure it all makes sense for them! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an incredible level of detail @joebingham-wk ... nice work!
Just have a couple of suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just spotted two more tiny things! Other than those and the image optimizations, this looks good to go!
0666550
to
82fa3c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Documentation looks awesome 🎉
- No broken links
+10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+10000
This was incredibly helpful for wrapping my JS components. The level of detail is great especially around the prop mapping for tricky types (like onChange), creating mixins for interfaces and testing. Thank you for writing this up!
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Motivation
The process for wrapping JS components in Dart is opaque despite us having the APIs available for it to be possible. We need a document that describes the process to make it feasible for consumers to adopt the pattern.
Changes
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: