-
Notifications
You must be signed in to change notification settings - Fork 78
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
Replace TokenSelector with the new version #16321
Conversation
Jenkins BuildsClick to see older builds (67)
|
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.
Looking good overall, gonna give it a test drive locally
Just some minor things inline
ui/app/AppLayouts/Wallet/controls/TokenSelectorCompactButton.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/controls/TokenSelectorCompactButton.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/controls/TokenSelectorCompactButton.qml
Outdated
Show resolved
Hide resolved
6da2cf4
to
6e468d5
Compare
6e468d5
to
478b475
Compare
478b475
to
6263bcf
Compare
6263bcf
to
26eb757
Compare
} | ||
|
||
function reset() { | ||
button.selected = false |
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.
Shouldn't you reset the searchableAssetsPanel.highlightedKey
here as well?
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.
Found an issue related to search probably:
Zaznam.obrazovky.z.2024-09-17.12-50-07.mp4
If you edit the search string (and even back to empty), then the list of assets displays again
@micieslak also, it would be nice to add/show some collectibles in SendModal under Storybook, hard to see whether it works correctly or not |
Another issue (specific to SwapModal); the search should not be persistent: Zaznam.obrazovky.z.2024-09-17.12-54-38.mp4 |
Could you provide the full video? I cannot reproduce, not sure what I'm missing. |
Here it is, didn't press or type anything, just switched the Assets/Collectibles tab: Zaznam.obrazovky.z.2024-09-17.13-36-53.mp4No warnings in console at all |
83a85db
to
13c42d3
Compare
No longer reproducible in |
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.
Looks good and works fine in the app; pls remove TokenSelectorView
(together with its SB page and QML test) completely instead of the changes done here, it's no longer used anywhere at all :)
a63ed71
to
fc39b41
Compare
|
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.
Nice job!
Works correctly everywhere now, great job! |
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.
8a5499c
to
ee3bf86
Compare
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.
All good! Scrolling fixed, the missing delegates were due to the SFPM changes (not being rebuilt on my side)
Thank you for testing! |
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.
Nice work! 👍 Minor questions/suggestions
|
||
closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutsideParent | ||
padding: 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.
WDYT of: onClosed: searchableAssetsPanel.clearSearch()
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.
The idea here is that the search is persistent and I think it shouldn't be
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 wasn't sure what approach is better and desired from design pov 🤔
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 it's uncommon to persist the filtering in the dropdown menu. But could be easily wrong 😄 Maybe Ben can help and we can create a task if any change is needed.
@benjthayer We have this kind of dropdown menu presented in the image below. It has the search bar that will filter the assets or collectibles. Should we persist the search string in between openings?
E.g. User searches for SNT
and decides to close the menu. Goes back to the dropdown and opens the menu. The search bar should be empty, or SNT
should still be there?
ui/app/AppLayouts/Wallet/panels/SelectParamsForBuyCryptoPanel.qml
Outdated
Show resolved
Hide resolved
font.pixelSize: 15 | ||
color: Theme.palette.baseColor1 | ||
text: ModelUtils.getByKey(holdingSelector.model, "tokensKey", holdingSelector.currentTokensKey, "symbol") | ||
function update() { |
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.
Could this be replaced with the ModelEntry? So instead of calling update manually to create a ModelEntry to select the asset entry to be selected and to configure the assetKey in the AssetSelectorCompact
. The AssetSelectorCompact
will update the internals if the key is valid.
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 that this and other problems are intended to be fixed here: #16211
d.isTokenSelected = true | ||
tokenSelectorButton.name = name | ||
tokenSelectorButton.icon = icon | ||
button.name = name |
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.
Isn't it an issue if the custom data is not available in the model?
I'm wondering why we have this setCustom
instead of selecting just the key from the outside and populate the button data with the data available from the model.
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.
This approach gives a bit more flexibility but I'm not fully sure now if it's needed (it's possible to set content not available in model).
At some point it was said that history of activities may refer to tokens that are not currently in the wallet. So if we want to open send/swap modal on it, me must have possibility to set entry not present in the model normally used for selection. But indeed I'm not sure if it's necessary. Anyways, it was motivation of that approach.
Thank you @alexjba for comments, please take a look at responses to decide with ones require some further adjustments. |
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.
Nice! Thank you 🍻
- clear search on close (AssetSelectorCompact) - sectionProperty removed - highlighting fixed in TokenSelectorPanel - setCustom renamed to setSelection - test data moved into Component object
@alexjba I added some fxes/amendments according to your suggestion:
|
What does the PR do
TokenSelector
withAssetSelector
inSwapModal
TokenSelector
withAssetSelectorCompact
inBuyCryptoModal
SwapModal
TokenSelector
and renameTokenSelectorNew
intoTokenSelector
. It requires migration of existing tests to the new components with more focus on testing in separation, with separate unit tests for subcomponents.Instead of single
TokenSelector
there are 3 small components composed of common sub-components:AssetSelector
(only assets, big button)AssetSelectorCompact
(only assets, small button)TokenSelectorNew
(both assets and collectibles in tabs, will be renamed toTokenSelector
)Closes: #16220
Closes: #16025
Closes: #16310
Affected areas
SwapModal
,BuyCryptoModal
Architecture compliance
My PR is consistent with this document: Architecture guidelines