-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat: add PromotedActionsBar #41972
feat: add PromotedActionsBar #41972
Conversation
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.
LGTM!
Design changes: @Expensify/design |
Can we use 8px as the gap between the buttons? I think we had this conversation previously when we were talking about the Search page buttons, and we seemed to like 8px as the standard gap. Does that sound familiar @Expensify/design? Or did we actually land on 12px? |
Sure, I can easily change it. Can you update the Figma designs first? |
Let's see what the @Expensify/design thinks first. |
Actually @kosmydel have you pulled main recently? I think what's happening is that these buttons might have an invisible wrapper that adds 1px of padding. So instead of 12px, your PR looks to have 14px gap between them: We ran into this in a previous PR and I am pretty sure we just got rid of that 1px padding wrapper as it was pretty much pointless. Can you take a look at that first, and then we can evaluate from there? |
I've updated this PR with a 50% maximum width for the actions bar, which was requested here. |
I think there might have been a misunderstanding. We don't want a max width of 50% on the entire bar/container, but rather, we just want each individual button to have a max-width of 50%. So that this way, if there is only one button present, it only takes up 50% of the width and it should be centered. |
Hmm, it is a screen from the Storybook. That's why it might appear strange. I'm attaching screenshots from RHP on the web. |
Okay cool, that looks great to me - thanks for confirming! |
I'm going to pull the latest main and test it :) |
|
||
return ( | ||
<View style={[styles.flexRow, styles.ph5, styles.mb5, styles.gap2, styles.mw100, styles.w100, styles.justifyContentCenter, containerStyle]}> | ||
{/* TODO: Remove the `Leave` button when @src/pages/ReportDetailsPage.tsx is updated */} |
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.
Do we have a WIP PR for this yet?
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.
cc @cdOut
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.
I’m handling this one in my ReportDetailsPage PR, I’ll link it here after I put up a draft PR for it later in the evening.
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.
Please check this PR we're already moving it there.
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.
changes LGTM, only comment is about the TODOs, which we'd ideally want to omit
Alright :) I've removed the TODOs. |
Rereviewing now |
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.
Code changes LGTM
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.
Code LGTM. Also updated the checklist with latest test videos to ensure all is well.
✋ 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/grgia in version: 1.4.77-11 🚀
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.78-5 🚀
|
Details
This PR:
ChatDetailsQuickActionsBar.tsx
toPromotedActionsBar.tsx
and makes thePromotedActionsBar
component generic and reusable.src/pages/ReportDetailsPage.tsx
component so it uses the new component.Fixed Issues
$ #42071
PROPOSAL: N/A
Tests
Pin
andShare
promoted actions are displayed and work as expected.Leave
button is a menu item and works as expected.src/stories/PromotedActionBar.stories.tsx
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Old versions:
Android: mWeb Chrome
Old versions:
iOS: Native
iOS: mWeb Safari
Old versions:
MacOS: Chrome / Safari
Old versions:
web.mov
MacOS: Desktop
Old versions: