feat: wallet select address screen UI in empty state #17248
Conversation
Jenkins BuildsClick to see older builds (75)
|
briansztamfater
force-pushed
the
feat/wallet-send-empty-state-ui
branch
6 times, most recently
from
5616c32 to
cda6d93
Compare
briansztamfater
changed the title
briansztamfater
requested review from
J-Son89,
ilmotta,
rasom,
ulisesmac,
smohamedjavid and
OmarBasem
| input-value (atom "") | ||
| selected-tab (reagent/atom (:id (first tabs-data)))] | ||
| (fn [{:keys [theme]}] | ||
| [rn/scroll-view |
There was a problem hiding this comment.
I believe that we don't need the scroll view as the root/parent, as only the content inside the tabs should be scrollable. WDYT?
There was a problem hiding this comment.
What is this scroll view for?
I believe the scrollable content would the FlatList of the currently focused tab, and its header would be the part above the list that needs to scroll as well
There was a problem hiding this comment.
Scroll is disabled in the ScrollView, it is just used to dismiss the keyboard when touching outside of the address input
| (when-not (empty? @clipboard) | ||
| (on-change @clipboard) | ||
| (reset! value @clipboard))) | ||
| (clipboard/get-string |
| @@ -166,7 +167,8 @@ | |||
| ;; from ScrollView (e.g. FlatList). There are open issues, here's | |||
| ;; just one about this topic: | |||
| ;; https://github.com/facebook/react-native/issues/31218 | |||
| :content-container-style {:padding-top (dec unread-count-offset)} | |||
| :content-container-style (merge content-style | |||
There was a problem hiding this comment.
assoc is a good choice here.
There was a problem hiding this comment.
should we call this prop container-style as it's essentially the outer later? 🤔
There was a problem hiding this comment.
content-container-style is most accurate actually, but we can simplify with container-style yes
There was a problem hiding this comment.
Well whatever is most accurate, container-style is what we're referring to add to the most outer view to add margin etc,
| [rn/view {:style style/account-switcher-placeholder}] | ||
| [rn/touchable-opacity | ||
| {:style style/account-switcher-placeholder | ||
| :on-press #(js/alert "Not implemented yet")}] |
There was a problem hiding this comment.
this should just take an on-press and the not implemented should be set from the app code 👍
There was a problem hiding this comment.
Agree, but maybe is better to refactor once #16456 is done since it will need to be refactored anyway and use a dropdown variant with emoji
| {:margin-horizontal 20 | ||
| :margin-vertical 12}) | ||
|
|
||
| (defn divider |
There was a problem hiding this comment.
I think we might have a divider component? 🤔
| [rn/touchable-opacity | ||
| {:style style/account-switcher-placeholder | ||
| :on-press #(js/alert "Not implemented yet")}] |
There was a problem hiding this comment.
We agreed to use pressable instead of other touchable components.
@ilmotta should we add this to the guidelines? I see touchable-opacity still often being used in new PRs
There was a problem hiding this comment.
Agreed @OmarBasem. I noticed in the last what, one month?, we started to use pressable a lot more. I see 30+ usages already, and it looks like the team already agrees with this decision, so it's easy to add to the guidelines :)
This is a good example of using the :deprecated metadata in the touchable-opacity function. Could even be more effective than the guideline, or both tactics could be used of course.
(def ^{:deprecated "Superseded by react-native.core/pressable"} touchable-opacity
(reagent/adapt-react-class (.-TouchableOpacity ^js react-native)))
| input-value (atom "") | ||
| selected-tab (reagent/atom (:id (first tabs-data)))] | ||
| (fn [{:keys [theme]}] | ||
| [rn/scroll-view |
There was a problem hiding this comment.
What is this scroll view for?
I believe the scrollable content would the FlatList of the currently focused tab, and its header would be the part above the list that needs to scroll as well
There was a problem hiding this comment.
Nice work @briansztamfater! I focused my comments mostly on performance, but they're not blockers at all, also to start to raise awareness about some recurring perf problems I see in PRs.
| (clipboard/get-string | ||
| (fn [clipboard] | ||
| (when-not (empty? clipboard) | ||
| (on-change clipboard) |
There was a problem hiding this comment.
@briansztamfater, my feedback to this address-input component is not originating from your PR, but it may be helpful for the future nonetheless.
Especially for components that wrap text inputs we should be more careful about performance. Every time the value state changes, the whole address-input component is recomputed by Reagent, all functions in the let bindings are redefined, and so on. A good chunk of the hiccup tree doesn't need to be recomputed on every key up event.
It's easy to fix these things while we're writing/creating the code, but after it's merged probably nobody will refactor this, and these things all add up to a slow app. In an ideal world, we should be slicing hiccup more diligently to minimize recomputation because the default performance we get by ignoring this practice is not good, as we all know.
| [rn/touchable-opacity | ||
| {:style style/account-switcher-placeholder | ||
| :on-press #(js/alert "Not implemented yet")}] |
There was a problem hiding this comment.
Agreed @OmarBasem. I noticed in the last what, one month?, we started to use pressable a lot more. I see 30+ usages already, and it looks like the team already agrees with this decision, so it's easy to add to the guidelines :)
This is a good example of using the :deprecated metadata in the touchable-opacity function. Could even be more effective than the guideline, or both tactics could be used of course.
(def ^{:deprecated "Superseded by react-native.core/pressable"} touchable-opacity
(reagent/adapt-react-class (.-TouchableOpacity ^js react-native)))
briansztamfater
force-pushed
the
feat/wallet-send-empty-state-ui
branch
2 times, most recently
from
314be45 to
d171a56
Compare
| [{:keys [blur? padding-top padding-bottom theme]}] | ||
| {:border-color (if blur? | ||
| (colors/theme-colors colors/neutral-80-opa-5 colors/white-opa-5 theme) | ||
| (colors/theme-colors colors/neutral-10 colors/neutral-90 theme)) | ||
| :padding-top 12 | ||
| :padding-bottom 8 | ||
| :padding-top (when padding-top padding-top) | ||
| :padding-bottom (when padding-bottom padding-bottom) |
There was a problem hiding this comment.
@J-Son89 refactored divider component so padding top / bottom can be customized as it had some hardcoded values. There were no usage of this component as far as I could search (previous to integrating it on this PR as you suggested), so this shouldn't affect other screens or components.
There was a problem hiding this comment.
Nice one, curious though should it be margin instead? Perhaps it's all the same however 🤷♂️
There was a problem hiding this comment.
It think it is the same for the purpose of this view
There was a problem hiding this comment.
Oh actually, if this is an external prop passed in it's better to just use a 'container-style' prop that gets merged to this style object. It's what we are doing for all other quo2 components.
There was a problem hiding this comment.
Yep that sounds better, I updated with that approach
briansztamfater
force-pushed
the
feat/wallet-send-empty-state-ui
branch
2 times, most recently
from
3086515 to
72e2660
Compare
briansztamfater
force-pushed
the
feat/wallet-send-empty-state-ui
branch
from
72e2660 to
a396668
Compare
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/wallet-send-empty-state-ui
branch
from
a396668 to
1c54664
Compare
fixes #16853
fixes #16890
fixes #16889
fixes #16891
Summary
Implement Select Address screen UI in empty state
sendemotyuilight.mp4
sendemptyscreendark.mp4
Platforms
Steps to test
status: ready