Fix - Suggested response button text doesn't wrap correctly in Concierge chat#82113
Conversation
|
@abzokhattab 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] |
| primaryTextNumberOfLines={actionableButtonsNoLines} | ||
| styles={{ | ||
| text: [isConciergeOptions || actionContainsFollowUps ? styles.textAlignLeft : undefined], | ||
| text: [isConciergeOptions || actionContainsFollowUps ? [styles.textAlignLeft, styles.breakWord] : undefined], |
There was a problem hiding this comment.
❌ PERF-1 (docs)
Creating a new array inline within a component rendered by a list's renderItem forces React to treat the styles prop as changed on every render, preventing reconciliation optimizations.
Suggested fix: Define the combined styles array outside the component or memoize it:
// At the top of the component or outside
const conciergeTextStyles = [styles.textAlignLeft, styles.breakWord];
// Then use:
text: [isConciergeOptions || actionContainsFollowUps ? conciergeTextStyles : undefined],Or use a useMemo:
const textStyles = useMemo(
() => [isConciergeOptions || actionContainsFollowUps ? [styles.textAlignLeft, styles.breakWord] : undefined],
[isConciergeOptions, actionContainsFollowUps]
);
// Then use:
styles={{ text: textStyles, ... }}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Hi, does this need c+ review? |
|
I guess but I'd leave final word to @marcochavezf |
|
Yup, we'd still require review @abzokhattab 🙇🏽♂️ |
can u please invite me to a workspace that have this enabled? you can use this acc: if its not possible to invite me then can u enable it for this workspace: |
you can use any fake email |
trjExpensify
left a comment
There was a problem hiding this comment.
Let's have @Expensify/design review this PR.
|
Shall we run a test build? |
|
I'm having some trouble running the iOS build locally. I’ve been troubleshooting for a couple of hours with no luck. @marcochavezf Could you please trigger an ad-hoc build? It will be faster to run on the ad-hoc build than to spend more time |
|
🚧 @marcochavezf has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@dubielzyk-expensify that's a good catch - I have a feeling that feature (from when you create an expense via your personal space) is not the same UI component that is used for the Concierge auto responses... we should align them though. I am finding it nearly impossible to test the new buttons via other flows though as Concierge constantly categorizes things based on past activity. Any idea how to trigger the buttons reliably? |
You should be able to get the new ones on the onboarding for a new user that "Manages my team's expenses" and "1-10". Then you should get a welcome message w/ them in the You may need to use
|
|
yup, the @expensifail.com domain might not wor, instead you can use @dmb.fun and without an + e.g. "testAdmin123@dmb.fun" |
|
@jmusial can you please check this comment #82113 (comment) |
Hey @shawnborton yes, no all Concierge response buttons are styled the new way. Right now only actions with follow ups have a new look. The rest is to be changed later in a follow up. Right now the most reliable way of getting them it is to:
|
|
Agree with you about the green buttons. Everything else is looking how I would expect it to though 👍 |
|
Hmm. I can get onboard with not going green, but it feels a bit weird as it's the primary and only action. I guess they're far more visible now anyways so I guess I'm fine with keeping them the regular button color. |
Reviewer Checklist
Screenshots/Videos |
abzokhattab
left a comment
There was a problem hiding this comment.
Thanks ... the changes LGTM
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #81748 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Ah sorry for the confusion, I was only suggesting that we leave those patterns as they are and only change the style of the phrasal response buttons for now. |
|
Awesome, let's get this into final review then 🚢 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.3.22-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.22-4 🚀
|




















Explanation of Change
This pr changes how text is wrapped in followup buttons.
Fixed Issues
$ #81748
PROPOSAL:
Tests
Offline tests
N/A
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
MacOS: Chrome / Safari