-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: MI Assign activity screens (M2-7406) #534
Conversation
This pull request is automatically being deployed by Amplify Hosting (learn more). |
This change is required for banners that lack a close button to render correctly and accept custom icons.
This change required a markdown parser to be able to inject the label below any paragraphs that contain only media (images, videos, audio) and above the first paragraph that contains textual content. Also noticed that the `Banners` component was incorrectly placed in a scrollable container, making banner alerts (such as validation errors) not appear to users who have scrolled down on an item that has a long description.
d7b2d47
to
e56fd76
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.
Approved with some minor feedback
This is slightly unrelated to this PR, but I noticed that if I start a take now, both sets of UI show up
So we need to add the condition && activityListItem.targetSubject === null
to this if statement
mindlogger-web-refactor/src/widgets/ActivityGroups/ui/ActivityCard/index.tsx
Lines 147 to 156 in 26d0036
useOnceLayoutEffect(() => { | |
if ( | |
context.startActivityOrFlow && | |
((!isFlow && context.startActivityOrFlow === activityListItem.activityId) || | |
(isFlow && context.startActivityOrFlow === activityListItem.flowId)) | |
) { | |
// Pass `true` to ensure activity doesn't resume from a previous progress state. | |
onStartActivity(true); | |
} | |
}); |
I incorporated your suggestions, @sultanofcardio. Good catch on the interaction of Take Now with MI Assign. It's not just the UI issue you identified that was introduced with MI Assign – Take Now will actually no longer start unless you are assigned (manually or auto) to the activity as a self-report. Since Take Now should take precedence over MI Assign, however, this needs to be fixed. I've created M2-7796 to address this. |
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.
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.
Nice work @farmerpaul. Just one small comment but pre-approving.
Peer tested successfully aside from the badge and banner also showing on Take Now, as @sultanofcardio pointed out. The suggested solution in ActivityCard/index.tsx
seems to work, although I'm not entirely sure I understand how? If going with it, I suggest adding a comment so others coming across it understand why && activityListItem.targetSubject === null
is needed.
@mbanting incorporated your feedback! Thanks for the 👀 |
📝 Description
🔗 Jira Ticket M2-7406
Adds informative banners and labels to Activity screens when performing a multi-informant assignment.
About {{name}}
) below any media, and above the first line containing text content📸 Screenshots
🪤 Peer Testing
Note
This feature depends on the
enableActivityAssign
feature flag. This should already be enabled in dev/local environments. It would also be worth testing this with that feature flag disabled to confirm that existing behaviour is preserved with the MI Assign feature disabled, by addingfeatures.enableActivityAssign = false
prior to thereturn
statement here inuseFeatureFlags
.Please refer to ticket's AC for the main peer testing steps.
In addition, after completing a multi-informant assigned activity, confirm that the POST to the
answers
endpoint includes the appropriatetargetSubjectId
. Also confirm that for self-reports, that property is not included.