fix(SendModal): Bridge modal Simple mode's scroll is very clunky #15969
Conversation
caybro
requested review from
Khushboo-dev-cpp,
noeliaSD,
Cuteivist and
jrainville
Jenkins BuildsClick to see older builds (7)
|
There was a problem hiding this comment.
NetworksSimpleRoutingView is green, and the ListView inside is green:
And the scrolling here is totally external. The list is intended to be unrolled, with own vertical scrolling not used. So it seems that it can be simplified more, and instead of non-interactive list view it can be just repeater.
Quick check shows that then it works as before:
Maybe only spacing needs to be adjusted.
And horizontal scrolling should be never needed here. If texts are wider, should be just elided.
Please take a look, maybe I'm missing sth but it seems it can be simplified.
It would be nice to have storybook NetworksSimpleRoutingView but I don't insist, can be done in a next step along with removing dependency on store.
| function setRouteDisabledChains(chainId, disabled) { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
There is this big disclaimer on the top of that file:
There is also relevant point and rationale in the guide (I know, not published yet...).
The good thing is that stub is used only in SendModalPage. So my proposition is to do small step towards the expected structure and move that content to the SendModalPage and leave only empty stub here.
There was a problem hiding this comment.
Yeah the problem is though that without this stub store, the SendModal would be completely broken in StoryBook; there's a lot of code snippets that rely on the store methods to be there; without them, the JS code flow just breaks
There was a problem hiding this comment.
And as for the proposed change: this is just a bugfix PR (for 2.30 eventually) and I'd definitely postpone any refactorings for the start of the next milestone
There was a problem hiding this comment.
This is no risk at all, just cut from the stub and paste in SendModalPage:
0 impact on the app, 0 risk. All on SB side. But instead of adding piece of code violating agreements, it will be left in a bit better shape, and will prevent from further violations of that kind there :)
There was a problem hiding this comment.
Not that simple, I tried; it needs dozens of all those methods like setRouteDisabledChains() I just added above
There was a problem hiding this comment.
Ah got it now, will do; I thought you wanted me to remove the whole stub TransactionStore right now :)
There was a problem hiding this comment.
Ah got it now, will do; I thought you wanted me to remove the whole stub TransactionStore right now :)
Omg, it would be something 🤣
There was a problem hiding this comment.
I actually meant to move the function you added along with the rest of stuff and removing disclaimer and leaving only plaing QtObject {} there. Moving a lot but just cut/paste basically.
There was a problem hiding this comment.
I'm approving as it's kind of "optional" work here anyways...
Yeah you are right; there's definitely more scroll/list views than needed in the whole SendModal. I'll try to revert back to just using a Repeater here |
caybro
force-pushed
the
15902-bridge-modal-simple-modes-scroll-is-very-clunky
branch
2 times, most recently
from
466d8b0
to
ebf2e7e
Compare
|
@micieslak reverted the ListView to Column+Repeater; pls have another look |
- remove the nested unneeded ScrollView - cleanups and fixes in mocked models and stores to unbreak showing the networks/routing in storybook Fixes #15902
caybro
force-pushed
the
15902-bridge-modal-simple-modes-scroll-is-very-clunky
branch
from
ebf2e7e
to
346b33b
Compare
caybro
deleted the
15902-bridge-modal-simple-modes-scroll-is-very-clunky
branch
What does the PR do
Fixes #15902
Affected areas
SendModal, NetworksSimpleRoutingView
Screenshot of functionality (including design for comparison)
Zaznam.obrazovky.z.2024-08-02.11-03-33.webm