-
Notifications
You must be signed in to change notification settings - Fork 984
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
chore(wallet): feature flag graph and hide about action button #19832
Conversation
:height 130 | ||
:align-items :center | ||
:justify-content :center}) | ||
{:height 110 |
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.
moved this to the page using and passed via container-style - (also updated preview screen)
:justify-content :center | ||
:flex 1 | ||
:max-height 106}) | ||
(def inner-container |
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.
moved this to the page using and passed via container-style - (also updated preview screen)
src/status_im/feature_flags.cljs
Outdated
::community.edit-account-selection (enabled-in-env? :FLAG_EDIT_ACCOUNT_SELECTION_ENABLED) | ||
::wallet.contacts (enabled-in-env? :FLAG_CONTACTS_ENABLED)})) | ||
::wallet.remove-account (enabled-in-env? :FLAG_REMOVE_ACCOUNT_ENABLED) | ||
})) |
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.
alpha sorted ^
Jenkins BuildsClick to see older builds (13)
|
@@ -96,4 +96,4 @@ | |||
:customization-color customization-color | |||
:derivation-path path | |||
:keypair-name keypair-name | |||
:on-press #(js/alert "To be implemented")}])])) | |||
}])])) |
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.
Trailing
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.
fixed!
(def overview-container | ||
{:height 86}) | ||
{:height 82}) | ||
|
||
(def accounts-list | ||
{:padding-top 32 | ||
:padding-bottom 12 | ||
:max-height 112}) | ||
{:padding-top 8 | ||
:max-height 92}) |
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 curious about these height/padding changes. Is it from the designs?
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 checked everything with figma 1-1 and it was sitting right like this from what I could see.
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.
actually, I think I will double check this again. If anything we should not be setting max-height like this and using margins for the spacing instead. The height of the cards changes depending on the content used. Same for many of the components. Unfortunately for a lot of these quo components we were using height as margin but this is not desirable. Will revisit
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.
fixed this @OmarBasem - I instead use margin/padding to handle the spacing there so it is dynamic 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.
Thanks for checking and fixing this @J-Son89
Btw, I think the default value of flex-grow
is 0
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 think in most cases yes but here I think the wrapper component is doing something as I needed to add this.
src/status_im/feature_flags.cljs
Outdated
::community.edit-account-selection (enabled-in-env? :FLAG_EDIT_ACCOUNT_SELECTION_ENABLED) | ||
::wallet.contacts (enabled-in-env? :FLAG_CONTACTS_ENABLED)})) | ||
::wallet.remove-account (enabled-in-env? :FLAG_REMOVE_ACCOUNT_ENABLED) | ||
})) |
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.
Trailing
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.
fixed this, thanks!
ad88004
to
6625f89
Compare
adding skip qa as it's just a UI change to remove the graph. cc @status-im/mobile-qa |
In the Offsite it was agreed to feature flag the wallet graphs until a later release.
see the design file:
https://www.figma.com/file/xLs1KYmF4e6WwRTZVJKeUK/Descoped-Wallet?type=design&node-id=7503-243135&mode=design&t=dtWnJDJElRxhMoag-0
This pr hides the home page graph and the account page graph.
It also hides the options button on the account "about" tab.
Home Page:
Account Page:
About Tab: