-
Notifications
You must be signed in to change notification settings - Fork 13
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
tech(): refactor buildUiSchema Utils to not take EntityEditorOptions #1312
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1312 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 810 810
Lines 14734 14732 -2
Branches 2569 2570 +1
=========================================
- Hits 14734 14732 -2
☔ View full report in Codecov by Sentry. |
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.
Left a couple thoughts, but otherwise this looks good to me. Nice work!
|
||
export function getFeaturedImageUrl( | ||
options: EntityEditorOptions, | ||
view: IWithViewSettings, |
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.
wondering if we need to pass the full view in here or if we can just pass in a url
. Also wondering if this util could be generalized - instead of getFeaturedImageUrl
, could it just be getImageUrl
or getAuthedImageUrl
or something like that so that it would work for other editor image urls, not just featured image - there really isn't anything specific to featured image here.
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.
Good plan -- will refactor to getAuthedImageUrl and generalize
@@ -10,7 +10,7 @@ export type EditorOptions = EntityEditorOptions | CardEditorOptions; | |||
* | |||
* However, it must always have "type" on the options, even if not the entire entity. | |||
*/ | |||
export type EntityEditorOptions = HubEntity & Record<string, any>; | |||
export type EntityEditorOptions = Partial<HubEntity>; |
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.
Is the reason this needs to be a Partial
because IHubGroup
doesn't have location
? I think it would be preferable to make this stricter to expect the entire entity (which is what we're always providing anyways).
And then same with all of the respective EntityUiSchema...
args - would expect a full IHub<Entity>
rather than a partial.
But disregard if there's an underlying complication that makes this unfeasible.
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.
We can make EntityEditorOptions = HubEntity
without the Partial, but when we take the Partial off the buildUiSchema
args, then we start getting typing issues in the getEditorSchemas
logic... :( I'll take the Partial off of the EntityEditorOptions, but I think we may need to leave it for the buildUiSchema functions to make it work :/
Description:
Refactors the utils in buildUiSchema to take specific props rather than the entire entity, allowing EntityEditorOptions to be strongly typed.
Instructions for testing:
Closes Issues: 8221
Updated meaningful TSDoc to methods including Parameters and Returns, see Documentation Guide
used semantic commit messages
PR title follows semantic commit format (CRITICAL if the title is not in a semantic format, the release automation will not run!)
updated
peerDependencies
as needed. CRITICAL our automated release system can not be counted on to updatepeerDependencies
so we must do it manually in our PRs when needed. See the updating peerDependencies section of the release instructions for more details.