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

Fix scrollbar in Open Trades screen [1 XMR] #677

Closed
woodser opened this issue Sep 4, 2023 · 20 comments
Closed

Fix scrollbar in Open Trades screen [1 XMR] #677

woodser opened this issue Sep 4, 2023 · 20 comments
Assignees
Labels
💰bounty There is a bounty on this issue

Comments

@woodser
Copy link
Contributor

woodser commented Sep 4, 2023

This issue requests fixing the scrollbar to the Open Trades screen, which can cut off the button to send payment, especially with long payment methods like Pay by Mail or on low resolution screens.

For example:

image

image
@woodser woodser added the 💰bounty There is a bounty on this issue label Sep 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

There is a bounty on this issue, the amount is in the title. The reward will be awarded to the first person or group of people who resolves this issue.

If you are starting to work on this bounty, please write a comment, so that we can assign the issue to you. We expect contributors to provide a PR in a reasonable time frame or, in case of an extensive work, updates on their progresses. We will unassign the issue if we feel the assignee is not responsive or has abandoned the task.

Read the full conditions and details of our bounty system.

@woodser woodser changed the title Add scrollbar to Open Trades screen Add scrollbar to Open Trades screen [0.25 XMR] Sep 4, 2023
@woodser woodser changed the title Add scrollbar to Open Trades screen [0.25 XMR] Fix scrollbar in Open Trades screen [0.25 XMR] Sep 4, 2023
@0xMimir
Copy link

0xMimir commented Sep 26, 2023

Is this issue still open, because I see that #673 has been mered

@woodser
Copy link
Contributor Author

woodser commented Sep 26, 2023

Yes this issue still applies.

@SweetBearKiller
Copy link

Hello,
I'd like to have a try at this issue :)

@woodser
Copy link
Contributor Author

woodser commented Nov 19, 2023

Ok, I've assigned it to you.

@SweetBearKiller
Copy link

I don't have the time needed to do this anymore.
You can take a look here for what i had done so far : e785272
That worked fine however I had some weird problems where the scrollbar only appeared on the first compilation after local repo init and that remained to be investigated.

@SweetBearKiller SweetBearKiller removed their assignment Dec 5, 2023
@preland
Copy link
Contributor

preland commented Jan 16, 2024

I can replicate the behavior described with that fix

I think it has something to do with the footer being given a minimum height at runtime, but not at initialization. Because of this, it will scroll at first, but will increase the size of the footer upon reload. I was only able to figure this out because when I first compiled the fix, I had manually set the size of the window to be a little short, and the scrollbar showed up at first, but going back to it instantly forced the size of the window to become larger and removed the scrollbar. It rarely, if ever, showed up again on further launches

Very weird behavior indeed

Just so I am clear on what is wanted: Do you want a scrollbar for the entire window (what SweetBearKiller did) or a scrollbar just for the trade information section?

@woodser
Copy link
Contributor Author

woodser commented Jan 16, 2024

I think ideally the scrollbar would be only for the trade information section, since the trades already have a scrollbar, and so that would create a nested scrollbar with the entire window.

But something is better than nothing, since this is a usability issue which is still present.

Very happy to have it looked at. Thanks.

@preland
Copy link
Contributor

preland commented Jan 17, 2024

It appears this is a byproduct of the app not scaling very well with differing resolutions; this seems to be an issue with all of the pages, though the open trades menu is the most egregious example

Here are some resolution examples:

1650x1050:
Screenshot 2024-01-17 at 10 54 01 AM
1280x960:
Screenshot 2024-01-17 at 10 54 27 AM
640x480:
Screenshot 2024-01-17 at 10 55 00 AM

5556x2568:
Screenshot 2024-01-17 at 10 55 17 AM

The UI appears to look best in ~1080p resolution with a ~16:9 aspect ratio. Even at 1440p, there begins to be scaling issues.

It appears that the trade info section was already a scrollarea, but the scrollbar had been set to never appear. Changing this causes it to appear, but it will simply scale with the trade-info area, even if extra info is off-screen.

@preland
Copy link
Contributor

preland commented Jan 20, 2024

An update on what I’ve been doing: I’m currently working on changing the UI to adapt better to differing resolutions and aspect ratios, with the end goal to get the lowest resolution image above to present (by default) similar to the highest resolution image.

You can check on my progress on the master branch of my fork if you are ever curious (I’ll commit whatever I’ve gotten finished every day or so regardless of the current state of the project; currently the ui is incredibly buggy because the dynamic scaling hasn’t been implemented yet for the main “building blocks” of the ui)

TL;DR I probably could’ve fixed this issue by manually changing the display type on this specific screen to hyper-specifically fix this issue, but the main issue at hand would keep showing up as the app enters production.

@woodser
Copy link
Contributor Author

woodser commented Jan 20, 2024

If that proves to be a rabbit hole, some hacky fix to this specific problem would be fine / best for basic functionality before release.

@preland
Copy link
Contributor

preland commented Jan 20, 2024

I’ll try and do that then. I’ll keep my current work on a separate branch, as it could allow for a lot of QOL things that aren’t currently possible (such as changing the text size)

@preland
Copy link
Contributor

preland commented Jan 20, 2024

That PR should fix the issue

@woodser woodser changed the title Fix scrollbar in Open Trades screen [0.25 XMR] Fix scrollbar in Open Trades screen [0.50 XMR] Feb 1, 2024
@woodser
Copy link
Contributor Author

woodser commented Feb 1, 2024

Bounty bumped to 0.50 XMR.

@preland
Copy link
Contributor

preland commented Feb 7, 2024

As I've been working on it, I've been having issues with repeatedly replicating the issue, especially when it comes to the width/height of the text boxes (In the case of the Pay By Mail form, that would be the address and additional info). My guess is that it is somewhat related to that, as well as something else weird going on when the submit payment button is combined with the form to create the full screen.

To make sure I'm not unnecessarily wasting time during development, is there any sort of "debug" build option that populates the tables with test data? In addition, is there a way to quickly view UI changes without fully recompiling the entire jar (for context, I have been using a local test network with manually inputted data, and have been recompiling with "make skip-tests" and executing with "make user{1 or 2}-desktop-local" whenever I make a change).

@woodser
Copy link
Contributor Author

woodser commented Feb 8, 2024

is there any sort of "debug" build option that populates the tables with test data?

Sure, what I do is run a test from the haveno-ts library which sets up the arbitrator and traders, funds their wallets, and initiates a bunch of trades with different currencies and payment methods.

  1. Delete existing app data for all local instances.
  2. Install haveno-ts so you can run tests, e.g. "Can get the version".
  3. Modify HavenoClient.test.ts so buyerSendsPayment: false. This will cause trades to skip completion so the UI can be developed.
  4. Run the test: "Can complete trades at the same time".

After this the desktop apps can be opened to verify default state.

is there a way to quickly view UI changes without fully recompiling the entire jar

There is also make haveno-apps to build the desktop apps even faster, but you may find make skip-tests is necessary to fully refresh, depending on the change (usually it's not necessary with UI changes).

@woodser woodser changed the title Fix scrollbar in Open Trades screen [0.50 XMR] Fix scrollbar in Open Trades screen [1 XMR] Feb 10, 2024
@woodser
Copy link
Contributor Author

woodser commented Feb 10, 2024

Bounty bumped to 1 XMR.

@woodser
Copy link
Contributor Author

woodser commented Feb 10, 2024

@preland I've assigned the issue to you. If you decide to not complete the issue, please let me know and I can un-assign for someone else.

@preland
Copy link
Contributor

preland commented Feb 10, 2024

Sounds good

@woodser
Copy link
Contributor Author

woodser commented Mar 20, 2024

Fixed with #773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💰bounty There is a bounty on this issue
Projects
None yet
Development

No branches or pull requests

4 participants