-
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
[Payment card / Subscription] Implement “Subscription details” section (UI) #42975
[Payment card / Subscription] Implement “Subscription details” section (UI) #42975
Conversation
src/pages/settings/Subscription/SubscriptionDetails/index.native.tsx
Outdated
Show resolved
Hide resolved
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.
Left comments.
src/pages/settings/Subscription/SubscriptionDetails/index.native.tsx
Outdated
Show resolved
Hide resolved
@fabioh8010 @VickyStash @rezkiy37 PR updated! I think I addressed everything |
Reviewer Checklist
Screenshots/Videos |
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.
@dubielzyk-expensify the border radius was 6px - in between 😄 The icons were indeed 20px, I changed them to 16x16, good catch! |
Good catch again, it's the wrong logo, can you send me a file with the light theme version of ExpensifyApproved? |
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.
Light logo change looks good! 🚀
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
||
const [privateSubscription] = useOnyx(ONYXKEYS.NVP_PRIVATE_SUBSCRIPTION); | ||
const [account] = useOnyx(ONYXKEYS.ACCOUNT); | ||
const [preferredTheme] = useOnyx(ONYXKEYS.PREFERRED_THEME); |
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.
Why did you use useOnyx
directly here instead of useTheme
? Theme context would probably be more performant in general, plus it's just more consistent across the codebase.
{!!account?.isApprovedAccountant || !!account?.isApprovedAccountantClient ? ( | ||
<View style={[styles.borderedContentCard, styles.p5, styles.mt5]}> | ||
<Icon | ||
src={Illustrations.ExpensifyApprovedLogo} |
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.
Didn't you forget to update this based on the theme?
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, actually @JKobrynski can you please create a follow-up PR to use themed illustrations and the useThemeIllustrations
hook for the ExpensifyApproved
logo? see https://github.com/Expensify/App/tree/32f176fe7734c27ee0b8e0256e8a8fd5a6ac237d/src/styles/theme/illustrations
feel free to tag me on the review
@roryabraham @JKobrynski I moved it to a new GH so it doesn't get lost forever: #43306 |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
Details
To access this newly created component, paste the following link into the browser
https://dev.new.expensify.com:8082/settings/subscription
or add this effect to
InitialSettingsPage.tsx
Fixed Issues
$ #38623
PROPOSAL: N/A
Tests
Prerequisites
To test this component you are going to need a Collect workspace or a Control workspace. To create either one go to https://staging.expensify.com and then to Settings -> Workspaces and click the New Workspace button. In the next step you can create a Collect/Control workspace, that will be available in new dot (on the same account) after login.
Offline tests
N/A
QA Steps
Same as Tests section above
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop