-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[CRITICAL] Implement <WorkspaceWorkflowsApprovalsCreatePage /> for Workflow Creation #46798
Changes from 48 commits
88e2519
b0878c5
66c99d3
a0c289b
11372db
d243bd5
d1f0dce
5108e12
062e45a
c0734c8
89f6ee0
8405648
3a39deb
c4da7e3
dbb4311
e92b654
d4ae460
77555f3
1c6ae4b
31d8863
faee4e3
ebd1e05
9fa7889
69c31e2
b67e8b8
ba8a3d3
755954c
603b6ff
230080e
54453fb
b9b7937
68d5caf
64e7f0b
cd36817
436457b
4c0078a
17cab91
f554da6
6707500
c38d5d7
d74ed36
f9ea365
9bde486
e732c6c
05c552f
914bb1f
30d8a6a
14f6eb2
69ab537
5f3f64f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import isEmpty from 'lodash/isEmpty'; | ||
import React from 'react'; | ||
import React, {useMemo} from 'react'; | ||
import type {StyleProp, ViewStyle} from 'react-native'; | ||
import {View} from 'react-native'; | ||
import useTheme from '@hooks/useTheme'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import Parser from '@libs/Parser'; | ||
import Icon from './Icon'; | ||
import * as Expensicons from './Icon/Expensicons'; | ||
import RenderHTML from './RenderHTML'; | ||
import Text from './Text'; | ||
|
||
type FormHelpMessageProps = { | ||
|
@@ -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; | ||
}; | ||
|
||
function FormHelpMessage({message = '', children, isError = true, style, shouldShowRedDotIndicator = true}: FormHelpMessageProps) { | ||
function FormHelpMessage({message = '', children, isError = true, style, shouldShowRedDotIndicator = true, shouldParseMessage = false}: FormHelpMessageProps) { | ||
const theme = useTheme(); | ||
const styles = useThemeStyles(); | ||
|
||
const processedText = useMemo(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed to |
||
if (typeof message !== 'string' || !shouldParseMessage) { | ||
return ''; | ||
} | ||
|
||
const replacedText = Parser.replace(message, {shouldEscapeText: false}); | ||
|
||
if (isError) { | ||
return `<alert-text>${replacedText}</alert-text>`; | ||
} | ||
|
||
return `<muted-text-label>${replacedText}</muted-text-label>`; | ||
}, [isError, message, shouldParseMessage]); | ||
|
||
if (isEmpty(message) && isEmpty(children)) { | ||
return null; | ||
} | ||
|
@@ -41,7 +61,7 @@ function FormHelpMessage({message = '', children, isError = true, style, shouldS | |
/> | ||
)} | ||
<View style={[styles.flex1, isError && shouldShowRedDotIndicator ? styles.ml2 : {}]}> | ||
{children ?? <Text style={[isError ? styles.formError : styles.formHelp, styles.mb0]}>{message}</Text>} | ||
{children ?? (shouldParseMessage ? <RenderHTML html={processedText} /> : <Text style={[isError ? styles.formError : styles.formHelp, styles.mb0]}>{message}</Text>)} | ||
</View> | ||
</View> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename these to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are other props in this component that are named similary, are we sure we want to name it differently?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do it in a separate PR 👍 |
||
|
||
/** Whether should render error text as HTML or as Text */ | ||
shouldParseErrorText?: boolean; | ||
|
||
/** Should check anonymous user in onPress function */ | ||
shouldCheckActionAllowedOnPress?: boolean; | ||
|
||
|
@@ -394,6 +400,8 @@ function MenuItem( | |
shouldBlockSelection = false, | ||
shouldParseTitle = false, | ||
shouldParseHelperText = false, | ||
shouldParseHintText = false, | ||
shouldParseErrorText = false, | ||
shouldCheckActionAllowedOnPress = true, | ||
onSecondaryInteraction, | ||
titleWithTooltips, | ||
|
@@ -802,6 +810,7 @@ function MenuItem( | |
shouldShowRedDotIndicator={!!shouldShowRedDotIndicator} | ||
message={errorText} | ||
style={[styles.menuItemError, errorTextStyle]} | ||
shouldParseMessage={shouldParseErrorText} | ||
/> | ||
)} | ||
{!!hintText && ( | ||
|
@@ -810,6 +819,7 @@ function MenuItem( | |
shouldShowRedDotIndicator={false} | ||
message={hintText} | ||
style={styles.menuItemError} | ||
shouldParseMessage={shouldParseHintText} | ||
/> | ||
)} | ||
</View> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import type { | |
AddressLineParams, | ||
AdminCanceledRequestParams, | ||
AlreadySignedInParams, | ||
ApprovalWorkflowErrorParams, | ||
ApprovedAmountParams, | ||
BeginningOfChatHistoryAdminRoomPartOneParams, | ||
BeginningOfChatHistoryAnnounceRoomPartOneParams, | ||
|
@@ -1305,14 +1306,21 @@ export default { | |
/* eslint-enable @typescript-eslint/naming-convention */ | ||
}, | ||
}, | ||
approverInMultipleWorkflows: ({name1, name2}: ApprovalWorkflowErrorParams) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blazejkustra Do you confirm this translation with the internal team before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, here's a doc |
||
`<strong>${name1}</strong> already approves reports to <strong>${name2}</strong> in a separate workflow. If you change this approval relationship, all other workflows will be updated.`, | ||
approverCircularReference: ({name1, name2}: ApprovalWorkflowErrorParams) => | ||
`<strong>${name1}</strong> already approves reports to <strong>${name2}</strong>. Please choose a different approver to avoid a circular workflow.`, | ||
}, | ||
workflowsDelayedSubmissionPage: { | ||
autoReportingErrorMessage: "Delayed submission couldn't be changed. Please try again or contact support.", | ||
autoReportingFrequencyErrorMessage: "Submission frequency couldn't be changed. Please try again or contact support.", | ||
monthlyOffsetErrorMessage: "Monthly frequency couldn't be changed. Please try again or contact support.", | ||
}, | ||
workflowsCreateApprovalsPage: { | ||
title: 'Add approval workflow', | ||
title: 'Confirm', | ||
header: 'Add more approvers and confirm.', | ||
additionalApprover: 'Additional approver', | ||
submitButton: 'Add workflow', | ||
}, | ||
workflowsEditApprovalsPage: { | ||
title: 'Edit approval workflow', | ||
|
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.
Could you please rename this to
shouldRenderErrorAsHTML
so it is more explicit?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.
Renamed to
shouldRenderMessageAsHTML
as this correlates better with othermessage
prop