[DEW] Add workspace settings UI for Dynamic External Workflow#75619
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
| > | ||
| <TNodeChildrenRenderer tnode={tnode} /> | ||
| </Text> | ||
| ); |
There was a problem hiding this comment.
Is there any reason we create a custom renderer?
There was a problem hiding this comment.
A cleaner approach instead of breaking it into multiple Text components where each part needs separate translation and then combining them together.
Without custom renderers, we'd need something like:
<View>
<Text>{translate('workflowsPage.partOne')}</Text>
<Text style={styles.link} onPress={navigateToAccountManager}>
{translate('workflowsPage.accountManagerText')}
</Text>
<Text>{translate('workflowsPage.partTwo')}</Text>
<Text style={styles.link} onPress={navigateToConcierge}>
{translate('workflowsPage.conciergeText')}
</Text>
<Text>{translate('workflowsPage.partThree')}</Text>
</View>Custom renderers keep the full sentence in one translation key with the links embedded as tags what do u think ?
also its reusable.
There was a problem hiding this comment.
Yeah, that makes sense. I also remember now there was a previous discussion on Slack to not separate the translation anymore.
melvin-bot
Bot
requested review from
bernhardoj
and removed request for
a team
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
Bot
requested review from
JmillsExpensify
and removed request for
a team
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4MacOS: Desktopdesktop.mp4 |
|
@abzokhattab the info icon doesn't show immediately. icon.mp4UPDATE: Ok, it's because we use |
|
Also, have you confirm the |
|
|
||
| if (isChildOfMutedTextLabel) { | ||
| linkStyle = [styles.link, {fontSize: styles.mutedNormalTextLabel.fontSize}]; | ||
| } |
There was a problem hiding this comment.
We already wrapped the text with <muted-text-label>, so in theory, we won't need to conditionally apply this style anymore. We can take the style prop and pass it to the Text style below.
There was a problem hiding this comment.
i am not sure if i get it right, you want to add a new prop style and provide the values from the parent? wouldnt that be a breaking change for all existing ConciergeLinkRenderer ?
There was a problem hiding this comment.
you want to add a new prop style and provide the values from the parent?
We won't provide the value. It will be provided by the renderer. It's like the parent style. So, in our case,
<muted-text-label> ... <concierge-link>Concierge</concierge-link>...</muted-text-label>
the concierge link will get the style from <muted-text-label>, and we will append it with linkStyle.
style={[style, linkStyle]}
wouldnt that be a breaking change for all existing ConciergeLinkRenderer ?
I think there is always a possibility of breaking change, but it should be the way of handling a custom renderer and cascading the style down to children. So, the current code is more like a workaround, and I'm afraid in the future, more workarounds will be added.
There was a problem hiding this comment.
yes but probably the fontSize is added for a reason if we remove that that might be a regression ... no ?
There was a problem hiding this comment.
Let's keep the HTMLEngineUtils.isChildOfRBR logic and remove our new logic only.
There was a problem hiding this comment.
if we bring back the old logic the font will be bigger:
There was a problem hiding this comment.
this is another approach yes, i just thought it would be safer to use the if condition as its done with the existing font
| let linkStyle: StyleProp<TextStyle> = styles.link; | ||
|
|
||
| if (isChildOfMutedTextLabel) { | ||
| linkStyle = [styles.link, {fontSize: styles.mutedNormalTextLabel.fontSize}]; |
There was a problem hiding this comment.
|
translation confirmation was requested here https://expensify.slack.com/archives/C01GTK53T8Q/p1763725701022619 |
|
Waiting for translation confirmation... |
|
The translation is confirmed. Can you please proceed with the checklist @bernhardoj |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.2.64-0 🚀
|
|
@francoisl did this get tested on staging yet? |
|
Yep looks good, I'll check it off the checklist. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.64-5 🚀
|
5 participants
Explanation of Change
In the workspace editor, add an extra explanation section when Dynamic External Workflow is enabled, and lock the Approvals toggle.
Fixed Issues
$ #75224
PROPOSAL:
Tests
Pre-requiste: Use a workspace with DEW enabled (approvalMode: DYNAMIC_EXTERNAL)
Offline tests
same as tests
QA Steps
same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-11-20.at.16.36.31.mov
MacOS: Desktop
Screen.Recording.2025-11-20.at.16.30.24.mov