New Swap Scene #4912
New Swap Scene #4912
Conversation
samholmes
force-pushed
the
sam/swap-scenes
branch
2 times, most recently
from
633d492
to
42cf80d
Compare
|
Changes are needed based on new designs from yesterday |
samholmes
force-pushed
the
sam/swap-scenes
branch
3 times, most recently
from
d9d99df
to
1fbc272
Compare
There was a problem hiding this comment.
I cannot review this atomic commit style, since the forest is hidden in the trees.
I would like to see:
- Use flexGrow over width for LineTextDivider
- Update text input colors (new commit, with the changes to edgeDark.tsx)
- Rename CryptoExchange* components
- Rename exchange scene route names
- Literally everything else, in one commit
Notice that the first commit for ifLoggedIn should not be included, for reasons given below.
Commit 3 should also include "Rename ExchangedFlipInput component to SwapInputCard", since it's all part of the same renaming deal.
I am going to review it locally in my diff tool, and then leave comments manually.
There was a problem hiding this comment.
This is all I have for now - I would like to see these major issues resolved (squashing commits and making the wallet mandatory in the swap input component) before I review further, because right now things are just too chaotic.
Also, please do not rename FlipInput2 to FlipInputOld. If you genuinely need to for the component for the swap scene, then call your fork SwapFlipInput, but FlipInput2 isn't deprecated, so it shouldn't have a name like "Old".
samholmes
force-pushed
the
sam/swap-scenes
branch
2 times, most recently
from
1353194
to
f2b6efe
Compare
There was a problem hiding this comment.
I didn't find much on this second round of reviews. I think it looks pretty reasonable now, and a lot of my confusions from before are gone - now that it's all one big commit, it's obvious which files are which, and what got renamed to what.
We will really have to test the FlipInput2 component, not only on the swap scene but in other places like the send scene where we also use it.
samholmes
force-pushed
the
sam/swap-scenes
branch
from
f2b6efe
to
9b020d5
Compare
src/components/themed/FlipInput2.tsx
Outdated
| @@ -261,6 +263,8 @@ export const FlipInput2 = React.forwardRef<FlipInputRef, Props>((props: Props, r | |||
| </EdgeTouchableOpacity> | |||
| </SideContainer> | |||
| </InputContainerView> | |||
|
|
|||
| {renderFooter ? renderFooter() : null} | |||
samholmes
force-pushed
the
sam/swap-scenes
branch
from
9b020d5
to
7a0391c
Compare
This property causes less issues with overflowing containers when the desing calls for a max width container.
Use the word "swap" instead of exchange for the scene component names and files. Avoid naming based on implementation details such as the use of "quotes", and instead use words that apply to the scene's semantics: "confirmation", "success", etc.
The max button will overlap the elements or space below the SwapInput. This is called for in the design for this scene.
samholmes
force-pushed
the
sam/swap-scenes
branch
from
7a0391c
to
5028093
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: