Asset transfer choose asset #520
Conversation
GhenadieVP
changed the title
GhenadieVP
changed the title
GhenadieVP
requested review from
kugel3 and
CyonAlexRDX
and removed request for
kugel3
| let heights = subviews.map { $0.sizeThatFits(.init(width: container.width, height: nil)).height } | ||
| let height: CGFloat = { | ||
| if !isExpanded { | ||
| return heights[0] + CGFloat(collapsedViewsCount - 1) * collapsedSpacing |
There was a problem hiding this comment.
Can we be sure that heights is nonEmpty here?
There was a problem hiding this comment.
yes, if subviews is nonEmpty, for which we guard above.
Sources/Features/AccountDetailsFeature/Coordinator/AccountDetails+View.swift
Outdated
Show resolved
Hide resolved
| /// Number of visible views in Collapsed state | ||
| var collapsedViewsCount: Int | ||
|
|
||
| public init(isExpanded: Bool, spacing: CGFloat = 10, collapsedSpacing: CGFloat = 10, collapsedViewsCount: Int = 3) { |
There was a problem hiding this comment.
I suppose collapsedSpacing is more like an offset? Btw, it looked like there was space between the subrows when expanded, I don't think that's the original design.
There was a problem hiding this comment.
at implementation level, yes, it is offset actually, used spacing to be aligned in terminology.
There is spacing between subrows though, but maybe not that much, will double-check,
There was a problem hiding this comment.
Well, it's an offset also visually, right? Isn't this the amount that stacked cards extend below the one above? A spacing is an empty space between two things.
...atures/AssetTransferFeature/Components/TransferAccountList/TransferAccountList+Reducer.swift
Outdated
Show resolved
Hide resolved
...atures/AssetTransferFeature/Components/TransferAccountList/TransferAccountList+Reducer.swift
Outdated
Show resolved
Hide resolved
...atures/AssetTransferFeature/Components/TransferAccountList/TransferAccountList+Reducer.swift
Outdated
Show resolved
Hide resolved
...atures/AssetTransferFeature/Components/TransferAccountList/TransferAccountList+Reducer.swift
Outdated
Show resolved
Hide resolved
| if let isSelected = state.isSelected { | ||
| state.isSelected?.toggle() | ||
| return .none | ||
| } |
There was a problem hiding this comment.
It looks like you're not using isSelected. And why do we only send the delegate action if isSelected is nil`? Rather than say false or nil?
There was a problem hiding this comment.
Is it using the same logic as in my recent PR? If isSelected is nil it means that it can't be toggled, only selected.
There was a problem hiding this comment.
yes, the same logic.
There was a problem hiding this comment.
alright, but then here it's enough to check if it's nil i guess
...setsFeature/Components/NonFungibleAssetList/Components/Row/NonFungbileAssetRow+Reducer.swift
Show resolved
Hide resolved
| if selectedAssets.contains(token) { | ||
| selectedAssets.remove(token) | ||
| } else { | ||
| selectedAssets.append(token) | ||
| } | ||
| state.selectedAssets = selectedAssets |
There was a problem hiding this comment.
This is such a common pattern, swift should have a .toggle(Element) method on Set or even Collection.
There was a problem hiding this comment.
Added as extension on IdentifiedArray for this particular case.
|
LGTM! Good job! |
Asset selection for Asset Transfer.