[Suggested Follow-ups][R1] Update the buttons styling & refactor#80782
Conversation
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.
|
melvin-bot
bot
requested review from
Krishna2323
and removed request for
a team
|
@Krishna2323 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
joekaufmanexpensify
and removed request for
a team
|
@Expensify/design please take a look @ the buttons and let me know your thoughts. For the sprint work we'll enable the new look just for the follow ups as we're worrying the testing may take too long if we enable it for all rn. Those will be enabled in a follow up. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import Onyx from 'react-native-onyx'; | ||
| import type {Ancestor} from '@libs/ReportUtils'; | ||
| // eslint-disable-next-line @dword-design/import-alias/prefer-alias | ||
| import {addComment, buildOptimisticResolvedFollowups} from '@userActions/Report'; |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
ESLint rule disablement lacks justification.
Reasoning: ESLint rule disables without justification can mask underlying issues and reduce code quality. Clear documentation ensures team members understand exceptions, promoting better maintainability.
Suggested fix: Add a comment explaining why the import-alias rule needs to be disabled here.
// eslint-disable-next-line @dword-design/import-alias/prefer-alias
// Direct import needed to avoid circular dependency with Report action file
import {addComment, buildOptimisticResolvedFollowups} from '@userActions/Report';Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@Krishna2323 this PR has a high priority, do you have some time to take look at it rn ? |
|
@codex review |
| paddingBottom: 8, | ||
| backgroundColor: 'transparent', | ||
| borderWidth: 1, | ||
| borderColor: theme.border, |
There was a problem hiding this comment.
when we're hovering the message change this to button press
Same as on the expense
There was a problem hiding this comment.
updated look cc @dubielzyk-expensify
Screen.Recording.2026-01-28.at.13.59.05.mov
I can help otherwise since I reviewed previous PR. |
Beamanator
requested review from
mkhutornyi
and removed request for
Krishna2323
|
@mkhutornyi yes please thanks so much for offering! @Krishna2323 sorry sir but we need quick reviews here and @mkhutornyi helped us review related PRs yesterday 🙏 |
|
@mkhutornyi go ahead plz, there still some burder changes coming after chat with the design team, but they should not change to much |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| const followupListMatch = html.match(followUpListRegex); | ||
| if (!followupListMatch) { | ||
| const doc = parseDocument(html); | ||
| const followupListElements = DomUtils.getElementsByTagName('followup-list', doc, true); |
There was a problem hiding this comment.
Will this work on native too?
There was a problem hiding this comment.
looked good when I was testing
| * @param timezoneParam - The user's timezone | ||
| * @param ancestors - Array of ancestor reports for proper threading | ||
| */ | ||
| function resolveSuggestedFollowup( |
There was a problem hiding this comment.
Now that this is moved to seprate file, can we fully deprecate this in actions/Report.ts?
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Ok, so I think it's expected since they are buttons and this is consistent with how buttons behave. I just got all "long" followups when testing :D |
ok so this is expected? btw let's not block on this |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #80686 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Ok, after a quick catchup with @dannymcclain: |
|
If left aligned, container should be stretched or not? Example: 
|
jmusial
dismissed stale reviews from thienlnam and joekaufmanexpensify
via
1933e22
This is a new look :) Those should be full width but text left aligned on mobile, not stretched on desktop 
|
|
Looks good mobile: 
web: 
|
|
@marcochavezf looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
I thought all the checks were passing, but it seems some of the test jobs were canceled. Re-running them here https://github.com/Expensify/App/actions/runs/21461693077 |
|
Tests are passing here, removing the Emergency label |
|
Discussed in Slack but I think we should update these button styles to look more buttony. Perhaps @dannymcclain and @dubielzyk-expensify can handle in person tomorrow? |
|
Yeah happy to! |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.3.11-16 🚀
|
|
@jmusial @thienlnam @marcochavezf Concierge is flagging expensifail account as test accounts and not getting any respones. Is this expected or can you please suggest any workaround for this prompt.ccg.mp4 |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
| text={props.shouldUseLocalization ? translate(item.text as TranslationPaths) : item.text} | ||
| medium | ||
| success={item.isPrimary} | ||
| innerStyles={props.styles?.button} |
11 participants
Explanation of Change
This is a follow up to #80539.
The PR includes refactor and follow up button styling changes.
Fixed Issues
$ #80686
PROPOSAL:
Tests
Pre requisites:
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
0089.release.1.follow.up.mov