-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Profile page UI updates #37285
Profile page UI updates #37285
Conversation
@allroundexperts 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] |
cc @shawnborton @luacmartins @Expensify/design |
Can you also confirm that you removed the rounded corners from the top header image? The whole card should have rounded corners, but that should be handled by the whole card wrapper. Then the header image that is inside of the wrapper wouldn't need rounded corners. |
confirmed |
@shawnborton replied to all your comments - all items you mentioned were fixed |
Reviewer Checklist
Screenshots/Videos |
@narefyev91 I think that button border color might still be a little off. This is what we're going for: The card background should be using |
can you please specify which color it should be - as i see in colors for dark/light - we do not have them @dannymcclain |
@shawnborton do you know where those variables are defined? We shouldn't need to be specifying colors based on light or dark, right? I thought we just set it to the variable and the theming took care of it? |
in code for card we use: |
This is how it will look like with productLight200 |
@narefyev91 I think there's some miscommunication here. We want the pencil button to remain as it was, using the standard button colors. The only thing we want to change is the border color on the outside of the button. We need to make this border color use the same color as the card BG colors. I believe that is something like product200. |
this is one with product200 border @shawnborton |
Wonderful, that appears to be correct. |
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.
Looks good on my end.
Sounds good. So let's just get some updated screenshots and we can get this thing merged. Thanks @narefyev91 |
done - all screenshots are 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.
Looks good!
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!
style={StyleSheet.flatten([styles.br4, styles.wAuto, styles.h68, imageStyle])} | ||
source={WorkspaceProfile} | ||
style={StyleSheet.flatten([styles.wAuto, styles.h68, imageStyle])} | ||
source={isDarkTheme ? WorkspaceProfile : WorkspaceProfileLight} |
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.
Just out of curiosity, is this how we handle all theming choices? I guess it works but that means we're not future proofing this to support more than two themes. @grgia do you have thoughts 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.
hmm that's a good point. We actually have useThemeIllustrations
hook which I think should be used in this case.
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.
ok - let me check and add a fix for that
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.
PR with fix - #37437
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.
@narefyev91 I already have a fix up
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.
Yes that's correct- thanks @luacmartins !
✋ 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/luacmartins in version: 1.4.45-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.45-6 🚀
|
Details
Updated Profile Workspace page after design audit
Fixed Issues
$ #35719
PROPOSAL: #35719
Tests
Offline tests
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 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