Skip to content
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

scrollbar fix, but better #773

Merged
merged 1 commit into from
Mar 20, 2024
Merged

scrollbar fix, but better #773

merged 1 commit into from
Mar 20, 2024

Conversation

preland
Copy link
Contributor

@preland preland commented Feb 5, 2024

This PR is a reimplementation of the scrollbar fix for the pending trades screen. It is a lot simpler than the earlier solution, while also maintaining higher functionality. The width issue should be addressed now, such that a horizontal scrollbar should no longer be necessary (at least for Pay By Mail; if another method requires horizontal scrolling it could be added back in). The width and height of the text sections now properly scale on different screen sizes.

The only issue I can find is a minor layout issue on Pay By Mail in certain conditions:

In some conditions, it will look fine:
Screenshot 2024-02-04 at 8 14 53 PM

In other conditions, it will look like this:
Screenshot 2024-02-04 at 8 15 01 PM

My guess is that is has something to do with the placeholder text being one column, while the form is two columns. The issue occurs consistently when moving from a placeholder view to the form view, while moving from a two column view (in my testing i used the Zelle form) to the form resulted in no changing. The issue only consistently reverts when moving from another tab back to the form. I was also able to get the issue to fix itself ~5% of the time by using an autoclicker and switching between tabs at around 20 cps, which I am unable to explain currently.

@preland preland requested a review from woodser as a code owner February 5, 2024 03:00
@woodser
Copy link
Contributor

woodser commented Feb 5, 2024

Thanks. Looking at the screenshot, the Pay by Mail text areas are also too large, compared to current master:

image

@preland
Copy link
Contributor Author

preland commented Feb 13, 2024

I believe that this should (finally) fix the issue. It seems that the issue all along was that the submit payment button wasn't having its height set properly (kinda), and so the HBox enclosing it would also not have the right height set, which would then cause the whole thing to render as if it had no height, despite it very clearly having height. Adding ~90 padding plus top-left aligning the button in the HBox fixes the issue.

@woodser
Copy link
Contributor

woodser commented Feb 13, 2024

It's looking much better. 🎉

Is it possible to remove this extra padding below the confirmation buttons?

image

That should negate the need for the scrollbar for most payment methods. Ideally there would be 0 extra padding under the buttons.

@preland
Copy link
Contributor Author

preland commented Feb 13, 2024

I tried to do that, but the scrollpane refuses to grow unless the hBox is given extra height. This could be done either using padding or by setting the minimum height of the hBox, which both give the same effect. The padding/height can be reduced further, but below 50 the scrollpane will begin cutting off the button again.

I used the padding instead of the height, as it makes it more clear that it is intended to be padding. I suspect that the issue is somehow related to the button implementation in JFoenix, although I am unsure.

@woodser
Copy link
Contributor

woodser commented Feb 13, 2024

Unfortunately the default view for most payment methods would have this unnecessary scrollbar with extra height at the bottom, whereas they didn't before, so it would be a step backwards in terms of overall user experience.

@preland
Copy link
Contributor Author

preland commented Feb 13, 2024

I’ll look and see what I can do for that

@woodser
Copy link
Contributor

woodser commented Feb 13, 2024

Thanks for your help.

@woodser
Copy link
Contributor

woodser commented Feb 18, 2024

Any luck @preland? Hoping this can make it in before release in the next few weeks.

@preland
Copy link
Contributor Author

preland commented Feb 18, 2024

I'm still working on it. I actually had it working almost perfectly (except there was a weird gap between the peer and main addresses) but it seems to have undone itself.

I'm not going to make any conjecture as to what's causing it, because I no longer think it's a single issue. I think I'll be able to get it done by this Friday

@woodser
Copy link
Contributor

woodser commented Feb 22, 2024

Happy to re-test if it's ready. Please let me know.

@preland
Copy link
Contributor Author

preland commented Feb 22, 2024

The last commit should be functioning as intended. Here are the things that (should) be working now:
-Vertical scrolling on the entire subview, instead of just on the form area (this fixes cases which would hide the matrix channel text below the support ticket button)
-Scrolling (finally) applies to the entire view with no (okay there is one, see below) janky workarounds
-The numerous layout issues I was having with TextAreas (not scaling properly, ignoring restraints, vertically centering even when explicitly told not to, etc etc) should be fixed by adding an invisible TextArea to the beginning of the subview. As far as I can tell, the TextArea doesn't mess with the layout of the form or block interaction with elements. This is necessary as the TextArea issues only occur to the very first instance of a TextArea within the view.
-(Extra one I forgot about originally): This also currently sets the maximum height of the table to 100; This can be increased if needed, but dynamically sizing the table will almost always result in scrolling being required on the lower view, regardless of screen size.

I think this is ready to be looked at. If this works properly, I can squash the commits.

@woodser
Copy link
Contributor

woodser commented Feb 25, 2024

I'm seeing a strip of blank padding added to the bottom of the screen, as shown by this cut off text and divider lines:

image image

Versus current master which stretches to the end of the screen:

image

@preland
Copy link
Contributor Author

preland commented Feb 27, 2024

This should fix that issue

@woodser
Copy link
Contributor

woodser commented Feb 28, 2024

It seems to be very close. The scrollbar appears correctly for a taller trade.

Only nit: when I click back to the first short offer again, the scrollbar remains vislbe, even though it didn't appear the first time and isn't necessary.

@preland
Copy link
Contributor Author

preland commented Feb 28, 2024

Interesting; I'm not able to reproduce that behavior myself, and looking into the code further, it appears that each element gets its own scrollview so this shouldn't be happening anyways.

Were the dimensions of the window default, or something else? Also, for future reference, what Java version are you using (I use 11.0.21-tem from sdkman).

@preland
Copy link
Contributor Author

preland commented Mar 3, 2024

Can you provide an update on the status of this issue?

@woodser
Copy link
Contributor

woodser commented Mar 10, 2024

The scrollbar is correctly appearing and disappearing after updating my JDK. Thanks.

My only request, if possible, is to restore previous behavior where the height of the trades list grows a little when the user grows the height of the application.

Otherwise with this PR, the trades list is fixed to always show only 2 trades, even if the user is in full screen mode and may have many trades.

@preland
Copy link
Contributor Author

preland commented Mar 10, 2024

The limit has now been removed

@woodser
Copy link
Contributor

woodser commented Mar 10, 2024

Seeing the scrollbar disappear with this change:

image

@preland
Copy link
Contributor Author

preland commented Mar 11, 2024

I am unable to reproduce this behavior; The following screenshots are from me building and running clean using java 21.0.2.fx-librca on MacOS 14.2.1:

Screenshot 2024-03-10 at 7 22 04 PM

This is with the initial window dimensions

Screenshot 2024-03-10 at 7 12 48 PM

This is with the minimum window dimensions

@woodser
Copy link
Contributor

woodser commented Mar 11, 2024

Oops, I must have rebuilt incorrectly. It's showing like your sceenshots now.

However, we want to preserve the same behavior that's currently on master and Bisq, where ~2 trades are shown by default, and the list height grows a little if the user increases the height of the application.

Otherwise, all this screen real estate is unused:

image

Hopefully it's possible without too much trouble.

@woodser
Copy link
Contributor

woodser commented Mar 18, 2024

@preland Any updates?

@preland
Copy link
Contributor Author

preland commented Mar 18, 2024

This commit should fix the aforementioned issue

@woodser
Copy link
Contributor

woodser commented Mar 19, 2024

Great, it's working.

Let's cap the max height to prevent unbound growoth of the trades list. I tested by setting the max height after the preferred height and it works:

tableView.setPrefHeight(100);
tableView.setMaxHeight(200);

So let's add that change and I think it's ready to merge. :)

@preland
Copy link
Contributor Author

preland commented Mar 19, 2024

The change has been made; should I squash commits?

@woodser
Copy link
Contributor

woodser commented Mar 19, 2024

The change has been made; should I squash commits?

Yes please.

@preland
Copy link
Contributor Author

preland commented Mar 19, 2024

Commits are now squashed

@woodser woodser merged commit 379ac6d into haveno-dex:master Mar 20, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants