feat: approver ux UI #609
feat: approver ux UI #609
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #609 +/- ##
==========================================
- Coverage 23.73% 23.72% -0.01%
==========================================
Files 165 165
Lines 6173 6178 +5
Branches 335 335
==========================================
+ Hits 1465 1466 +1
- Misses 4708 4712 +4
|
edgarkhanzadian
force-pushed
the
feat/approver-ux-ui
branch
2 times, most recently
from
f9b9a33 to
242164d
Compare
edgarkhanzadian
requested review from
kyranjamie,
pete-watters,
tigranpetrossian,
fbwoolf and
alter-eggo
apps/mobile/src/features/approver/components/approver-account-card.tsx
Outdated
Show resolved
Hide resolved
apps/mobile/src/features/approver/components/approver-account-card.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This looks great @edgarkhanzadian. I may have created some conflicts for you bc I didn't realize you were moving stuff arnd with the send form routes. Let me know if you need help resolving.
|
The highlighter code is 🔥 @edgarkhanzadian! |
| import { ScrollView } from 'react-native-gesture-handler'; | ||
|
|
||
| import { t } from '@lingui/macro'; | ||
|
|
||
| import { Highlighter, Prism, Text } from '@leather.io/ui/native'; | ||
|
|
||
| /* eslint-disable-next-line lingui/no-unlocalized-strings */ | ||
| const sampleCode = ` | ||
| ;; hello-world contract | ||
|
|
||
| (define-constant sender 'SZ2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQ9H6DPR) | ||
| (define-constant recipient 'SM2J6ZY48GV1EZ5V2V5RB9MP66SW86PYKKQVX8X0G) | ||
|
|
||
| (define-fungible-token novel-token-19) |
There was a problem hiding this comment.
Should we organise components elsewhere than approver. This component doesn't really have anything to do with it, and could just be a standalone component?
There was a problem hiding this comment.
I think it would be good to put it in the UI library
There was a problem hiding this comment.
Well i mean the main part of it is the Highlighter component that's already in the UI library. This one just adds a scrollview wrapper and a title that make it specific to approver. So it's probably fine for it to stay here.
| const [isDisplayingAdvancedView, setIsDisplayingAdvancedView] = useState(false); | ||
| const [actionBarHeight, setActionBarHeight] = useState(100); | ||
| const childRegister = useRegisterApproverChildren(); |
There was a problem hiding this comment.
Could this be all wraped into its own hook so we share it between web/native?
There was a problem hiding this comment.
I don't think it should, i use all of these hooks in one component here, but web uses some of these hooks separately.
| @@ -36,6 +36,7 @@ | |||
| "dependencies": { | |||
| "@crowdin/ota-client": "2.0.0", | |||
| "@expo/vector-icons": "14.0.0", | |||
| "@gorhom/bottom-sheet": "4.6.3", | |||
There was a problem hiding this comment.
I moved @gorhom/bottom-sheet dependancy into the UI library.
Is there some reason not to use standard Sheet? Maybe we can export BottomSheetScrollView from the UI library and import here to avoid extra dependancies?
The same for prism-react-renderer, if we can move that component into the library then we could save the import here
There was a problem hiding this comment.
Well i mean all of the dependencies are going to be included in the bundle anyway + syncpack syncs all of the dependency versions so we are not going to experience any version differences
| @@ -0,0 +1,25 @@ | |||
| import { AppRoutes } from '@/routes'; | |||
There was a problem hiding this comment.
I'm not sure about having state connected components under src/components
I would think they should be a feature?
There was a problem hiding this comment.
I wanted to ask here, this isn't how I've determined components vs features in the past, is this how others think about it? I've always thought, if a component is used in multiple features it should live outside the feature and be a shared component? 🤔 Would badges then be it's own feature, bc it seems odd now that the send feature is importing a component from the settings feature?
There was a problem hiding this comment.
Yeah, we have some dep cruiser rules for this in the extension we need to add to mobile...
There was a problem hiding this comment.
My understanding is similar but that if a shared component is connected to state, it then becomes a feature rather than a component. That's just how I have seen things done in the past generally.
I'm not sure if we can import one feature to another with those cruiser rules? Maybe they are enforcing a different setup we prefer.
Usually only dumb, non state connected components would go to src/components/ then state connected ones into src/features. If a component is used in multiple features:
- it could be made more dumb and receive props rather than hook into state
- if it needs to fetch state be promoted to a feature
apps/mobile/src/features/approver/components/approver-account-card.tsx
Outdated
Show resolved
Hide resolved
apps/mobile/src/features/approver/components/inputs-outputs-card.tsx
Outdated
Show resolved
Hide resolved
| export function SignPsbt() { | ||
| const theme = useTheme<Theme>(); | ||
|
|
||
| function getButtons(buttonState: 'start' | 'approving' | 'success') { |
There was a problem hiding this comment.
This could be it's own Approver.Action component rather than a function?
There was a problem hiding this comment.
Good idea! Will do now
There was a problem hiding this comment.
Great work here @edgarkhanzadian 👍
I added some organisation suggestions but really solid PR overall
edgarkhanzadian
force-pushed
the
feat/approver-ux-ui
branch
from
f470f14 to
987caea
Compare
Well probably too many changes for 1 PR but oh well. 😅
Initial route changed here to showcase the approver ux
approverUx.mov