[CRITICAL] Implement <WorkspaceWorkflowsApprovalsCreatePage /> for Workflow Creation #46798
Conversation
blazejkustra
changed the title
…approval-workflows/create-edit-screen
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@shawnborton Check this out when you have some time, the whole creation flow is ready 😄 |
|
@blazejkustra Should we hide the add approval button if the member list is empty? Screen.Recording.2024-08-13.at.15.13.26.movcc @Expensify/design |
Not sure about this, but it's a minor thing we can adjust later. Curious to see design team opinion on this (cc @shawnborton as you have a lot of context) |
🤔 How is the member list empty? Regardless, I think I'd be in favor of leaving the |
|
@dannymcclain If a member belongs to an approval workflow, we will not display this member in the new approval workflow. In this case, all members belong to the previous approval workflows, so there are no members in the member list anymore |
|
Ah thank you for explaining that! Makes perfect sense. @Expensify/design I wonder if in that case we should display an "empty state" when you click the I don't love the idea of that button just going away without any explanation. I don't know that any users will understand why that's happening. I think it'd be better to somehow tell the user "Hey, all your members are part of approval workflows already! Right on. Everyone is getting approved. Way to go." |
src/components/FormHelpMessage.tsx
Outdated
| @@ -23,11 +25,29 @@ type FormHelpMessageProps = { | |||
|
|
|||
| /** Whether to show dot indicator */ | |||
| shouldShowRedDotIndicator?: boolean; | |||
|
|
|||
| /** Whether should render error text as HTML or as Text */ | |||
| shouldParseMessage?: boolean; | |||
There was a problem hiding this comment.
Could you please rename this to shouldRenderErrorAsHTML so it is more explicit?
There was a problem hiding this comment.
Renamed to shouldRenderMessageAsHTML as this correlates better with other message prop
src/components/FormHelpMessage.tsx
Outdated
| const theme = useTheme(); | ||
| const styles = useThemeStyles(); | ||
|
|
||
| const processedText = useMemo(() => { |
There was a problem hiding this comment.
Renamed to HTMLMessage (as this is not a function but a memo)
src/components/MenuItem.tsx
Outdated
| @@ -261,6 +261,12 @@ type MenuItemBaseProps = { | |||
| /** Whether should render helper text as HTML or as Text */ | |||
| shouldParseHelperText?: boolean; | |||
|
|
|||
| /** Whether should render hint text as HTML or as Text */ | |||
| shouldParseHintText?: boolean; | |||
There was a problem hiding this comment.
Please rename these to shouldRenderHintAsHTML and shouldRenderErrorAsHTML
There was a problem hiding this comment.
There are other props in this component that are named similary, are we sure we want to name it differently?
shouldParseTitle = false,
shouldParseHelperText = false,
shouldParseHintText = false,
shouldParseErrorText = false,
There was a problem hiding this comment.
Yes, I think having them more explicit is better than consistency. In fact, I'd suggest renaming all of them in order to really clean it up.
As an external consumer of the component, I wouldn't really have any idea what "parse text" implies and I'd have to dig into the component and look at the code to try to understand it.
There was a problem hiding this comment.
but to be clear, I'm not asking you to rename the other props in this PR :D Maybe you or I could do a followup PR to clean those up later.
There was a problem hiding this comment.
I'll do it in a separate PR 👍
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.20-0 🚀
|
|
@DylanDylann @tgolen Do we need to use specific account to QA this or its part of BETA and should be enabled for Expensifail accounts? |
@mvtglobally Yes, it's behind a flag, specifically |
| const approverHintMessage = useCallback( | ||
| (approver: Approver | undefined, approverIndex: number) => { | ||
| const previousApprover = approvalWorkflow.approvers.slice(0, approverIndex).filter(Boolean).at(-1); | ||
| if (approver?.isInMultipleWorkflows && approver.email === previousApprover?.forwardsTo) { |
There was a problem hiding this comment.
@blazejkustra It fails in this case. We shouldn't display hint text
There was a problem hiding this comment.
It is a valid point @DylanDylann, I think we should change the hint message to something simpler as the current message is making the display logic impossible (there are too many cases which we can't handle with just one hint message).
@JmillsExpensify what do you think to change the hint message to something more generic?
[email protected] is already used in a separate workflow. If you change this approval relationship, all other workflows will be updated.
|
FYI I believe this was deployed to prod yesterday, from this checklist - #47356 |
| const errorText = approverErrorMessage(approver, approverIndex); | ||
| const hintText = !errorText && approverHintMessage(approver, approverIndex); | ||
| return ( | ||
| <MenuItemWithTopDescription |
There was a problem hiding this comment.
As it's allowed to edit approver, this item should be wrapped with OfflineWithFeedback. This causes a regression #50477
Details
Link to the doc.
UI changes
Members:
<MenuItemWithTopDescription />that displays all currently selected users for a given approval workflow.Approvers:
The whole section is dynamic as there is no limit on how many approvers there are.
Always one empty input is displayed to allow for adding additional approvers.
When there are more than one approver these items will be enumerated,
toLocaleOrdinalwill generate the ordinal based on the index of an approver.Onyx actions:
Clicking the back button will use the
clearApprovalWorkflowaction and navigate back to the workflows page (‘settings/workspaces/:policyID/workflows’).Clicking on the “Add workflow” button will trigger
createApprovalWorkflowaction and navigate back to the workflows page (‘settings/workspaces/:policyID/workflows’).Edge cases
In case one of the approvers already has an existing
forwardsToin place, we’ll populate that relationship when creating/editing an approval workflow - this will be implemented insetApprovalWorkflowApproverwhen selecting an approver. Hint text will be displayed based onisApproverInMultipleWorkflowssaved in the Onyx for each Approver. This utilizes hintText prop from<MenuItem />component.If the same person is chosen twice in one workflow, a dot indicator with an error message will be displayed to the user. This utilizes
errorTextprop from<MenuItem />component.Fixed Issues
$ #45954
PROPOSAL: N/A
Tests
More featuressettings page)canUseWorkflowsAdvancedApprovalfunction to return true (or usecanUseWorkflowsAdvancedApprovalbeta)Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.webm
Android: mWeb Chrome
android-web.webm
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov