-
Notifications
You must be signed in to change notification settings - Fork 19
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
[To Main] Feature - DESENG-667: Add engagement configuration summary #2577
[To Main] Feature - DESENG-667: Add engagement configuration summary #2577
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2577 +/- ##
=======================================
Coverage 76.04% 76.04%
=======================================
Files 609 609
Lines 22080 22083 +3
Branches 1785 1785
=======================================
+ Hits 16790 16793 +3
Misses 5026 5026
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks for this quick work Nat! Could you please just review that screen reader users will have an okay time with the tooltip button for copying the URL? Ideally they'd get all the same feedback as sighted users but if not we can just make sure the button instructions are good.
Also, maybe it'd be a good idea for us all to start communicating more about adding net-new components and changing app infrastructure. I'm trying to be mindful of managing complexity with this app. Could you maybe see if you could synthesize ConfigWizard and CreationWizard? I won't be insistent on it since you know the code best but we need to try to be economical whenever we can.
met-web/src/components/engagement/admin/config/EngagementUpdateAction.tsx
Show resolved
Hide resolved
@@ -0,0 +1,147 @@ | |||
import React, { Suspense } from 'react'; |
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.
There looks to be a lot of very similar code to that in CreationWizard
. Was there a reason we needed to create a net-new component for this? I see that you made it a Modal - does it need to be one or could we have just sent folks back to the CreationWizard?
met-web/src/components/engagement/admin/config/EngagementUpdateAction.tsx
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Issue #: https://citz-gdx.atlassian.net/browse/DESENG-667
Description of changes:
Feature Add engagement configuration summary 🎟️ DESENG-667
User Guide update ticket (if applicable):