-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Feature/35711 header invite remove button redesign #36220
Feature/35711 header invite remove button redesign #36220
Conversation
@Santhosh-Sellavel is assigned to the parent issue of this PR, so I'm going to assign him for the C+ review. Santhosh, if you can't get to this today, please let me know! |
Oh hm, the |
cc @Expensify/design for visibility on this one too. |
There's a note in the OP on this one:
|
Ah, got it - sorry for missing that. If we're keeping the Remove button for now, I think on mobile we should make it so that invite and remove just take up equal 50/50 widths in that button area. Thoughts on something like that? Otherwise it feels odd that one button is so much larger than the other in that view. |
That makes sense to me, I do agree it looks pretty weird right now haha. So then in #35713 we update it. |
Taking over! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-13.at.2.35.55.AM.movAndroid: mWeb ChromeScreen.Recording.2024-02-13.at.2.34.56.AM.moviOS: NativeScreen.Recording.2024-02-13.at.2.33.56.AM.moviOS: mWeb SafariScreen.Recording.2024-02-13.at.2.32.21.AM.movMacOS: Chrome / SafariScreen.Recording.2024-02-13.at.2.25.53.AM.movMacOS: DesktopScreen.Recording.2024-02-13.at.2.27.46.AM.mov |
@trjExpensify Even though we're keeping the remove button, the text inside the |
@shawnborton The distance between the search placeholder and the add member button seems to be too less on mobile. Can you please confirm if this is per design? |
There should be 12px gap between header area and content below. |
When the search is active (the placeholder is at the top), I think it's not |
Okay, confirmed. When the search bar is active, the distance between the buttons and the placeholder becomes |
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.
One minor change needed. Rest looks good!
I've approved this early since the change is super minor and I don't want to block this PR because of that.
🎯 @allroundexperts, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #36382. |
@shawnborton @trjExpensify @allroundexperts I've just pushed a commit that fixes all the mobile issues mentioned above: 12px bottom margin added, buttons are now distributed evenly and the text is center aligned. |
@burczu there's a conflict that needs to be resolved. Other than that LGTM 👍 |
@lakchote Git says there is one change request from @luacmartins but I've addressed it already... It seems it blocks merging - can we overcome it somehow? |
Changes have been addressed since last review by Carlos, and Carlos is OOO
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@burczu thanks! Do you have an updated screenshot I can take a look at? |
@shawnborton It looks like below now: |
Perfect, thanks! |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.41-12 🚀
|
Details
This PR introduces a new icon next to the Workspace Members Page's title. It also moves the Invite and Remove buttons to the header (web/desktop) and changes the design of the Invite button (screenshot of the expected design below).
Note: I'm keeping the Remove button here, to not break the key functionality of the members page. The button will be removed in #35713 while changing the removing functionality.
Fixed Issues
$ #35711
PROPOSAL: https://docs.google.com/document/d/1gk3xqOs7epMbUrSSiX8K7YcqfPLVgqEos0sf-D-GMDA/edit
Tests
Offline tests
Same as above.
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop