-
Notifications
You must be signed in to change notification settings - Fork 14
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
DRYD-1351: Rework Create Page #246
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v10.x #246 +/- ##
==========================================
- Coverage 98.17% 98.08% -0.09%
==========================================
Files 597 599 +2
Lines 12739 12821 +82
Branches 2602 2614 +12
==========================================
+ Hits 12507 12576 +69
- Misses 229 242 +13
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@mikejritter I ran this code locally. In the UI, under Create New -> Procedures I see a new subsection:
corresponding to the output from the {
"document": {
"@name": "servicegroups",
"ns2:servicegroups_common": {
"@xmlns:ns2": "http://collectionspace.org/services/servicegroup",
"name": "procedure",
"uri": "/servicegroups/procedure",
"hasDocTypes": {
"hasDocType": [
"Dutyofcare",
"SummaryDocumentation",
"Consultation",
"NagpraInventory",
"RepatriationRequest"
]
}
}
}
}
which I take it is the expected behavior. So, looks good. |
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.
I don't speak a lot of React but everything looks reasonable to me and poking around in the UI it seems fine as far as that goes.
What does this do?
Why are we doing this? (with JIRA link)
Jira: https://collectionspace.atlassian.net/browse/DRYD-1351
With the new procedures added for 8.1, the Create New page has become crowded. This groups related procedures together based on their service tags in order to add some organization.
How should this be tested? Do these changes have associated tests?
npm run devserver
Dependencies for merging? Releasing to production?
The reducer for the service tag really should handle the case of multiple tags for a single record type. Currently there aren't any records which do this but still, it's something to consider.
The style of the redux components is old as well, and could be updated to be more modern. I chose not to do any redux updates here but it's something worth discussing.
I also meant to check if the panels in the CreatePage can be memoized. They shouldn't have any updates over a session so they should only need to be rendered once.
Has the application documentation been updated for these changes?
No. Docs need to be added for the additional configuration for service tags messages and sortOrder.
Did someone actually run this code to verify it works?
@mikejritter ran locally