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

List offers #1990

Closed
wants to merge 3 commits into from
Closed

List offers #1990

wants to merge 3 commits into from

Conversation

DrColver
Copy link
Contributor

@DrColver DrColver commented Apr 4, 2024

Add checkbox to show offers as list instead of as chat

It's quite hard to get an overview of the offers in the chats. This draft PR adds an option to display the offers in a compact table that can be sorted on price or reputation for example.

Are there some arguments against showing the offers as a table? It seems like a natural way to get some order, but it does lose some of the community feel of the chat.

image

image

@HenrikJannsen HenrikJannsen requested a review from axpoems April 4, 2024 17:06
@HenrikJannsen
Copy link
Contributor

See also #1972

@DrColver
Copy link
Contributor Author

DrColver commented Apr 5, 2024

I wasn't aware of this issue when I implemented this table view. I was just bothered by the lack of overview. This PR seems to implement the requested feature.

Some points regarding the code:

  • The ChatOfferTable is using the same model and controller as the chat view since it's just a different way to display the same data.
  • Columns in the table have not been thoroughly considered, just some basic ones that came to mind.
  • The price comparison handling for offers is very bothersome to work with. Some convenience class around that is likely needed, both to make code more readable, and to make the usage of prices more uniform within the app. The different options of Percentage above vs Fixed vs Market make sense for the user but will lead to confusion if not clearly annotated if mixed within the order book.
  • Binding sorting of a table columns to the associated SordedList seems like it could be done better.

DrColver added 2 commits April 5, 2024 17:08
Add checkbox to show offers as list instead of as chat
@axpoems
Copy link
Contributor

axpoems commented Apr 5, 2024

First of all, I appreciate your proactivity. However, as you already are aware, there was an issue open up for discussion regarding this implementation.

Setting aside the specifics of the issue, I'd like to highlight some of the utility of Bisq GH issues. Primarily, it serves as a means to communicate problems or proposed features to both the community and developers and discuss them before development. We also use tagging to prioritise them, signalling what should be worked on first. Additionally, assigning to someone helps prevent overlapping efforts. As you may have noticed, I was assigned to this task and was already working on it.

So in the future, especially when implementing significant UI changes, I'd recommend that you create and/or request assignment of the issue beforehand. And if UI related, use mock-ups (or express with details) upfront to convey the idea. This is important to maintain a coherent UX and UI design in the Bisq 2 app.

Answering your question in the PR description above, yes, there are arguments against just adding a table view just like the one you propose. Having said that, are you able to upload a complete mock-up of your proposal for this feature?

@DrColver
Copy link
Contributor Author

DrColver commented Apr 5, 2024

@axpoems I understand your annoyance at this proposal since there was now an issue about it and that you have started working on it. Consider this a discussion basis rather than a code PR to merge. I don't have a mock up, just the code, which has helped me consider the issues concretely.

I saw there was a discussion on the wording and color of take offer buttons so I figured the same should be used here once that's completed. There is the confusion on perspective on sell vs buy. The drop down filter for what I want to do and the offers from the offer creators perspective.

The table content could also include method, ie main net or lightning.

I think the look should be basically the same as other tables, which is what I used here, but UX is not my specialty.

@axpoems
Copy link
Contributor

axpoems commented Apr 6, 2024

There's no annoyance, I just gave an exposition of the dynamics occurring within Bisq development. As I said, I appreciate the proactivity, and if in a coordinated way, even better. I consider my comments the beginning of the discussion.

I see some issues with this design proposal. I will look into those and to the solution as whole and come back with specifics.

@DrColver
Copy link
Contributor Author

DrColver commented Apr 6, 2024

I likely won't have much time to work on this over the coming week or two.

As you've noted, there are issues. If you've already started on an implementation that fits better with the framework, you might just want to use some of the ideas from here.

After using the table view I'm convinced it's needed as a complement to the chat.

@axpoems
Copy link
Contributor

axpoems commented Apr 6, 2024

Thanks for the notice.
I agree that this will be a useful complement to the chat, but that is exactly the UX problem to solve: how to make the table complement the chat (and not replace it completely).

@DrColver
Copy link
Contributor Author

@axpoems I'm looking at this again, and from what I could see you haven't yet implemented the offers list.

I thought a bit more on how to complement the chat rather than having an either or choice of list or chat.

One way could be to do a popup window with the list. That would be more like some traditional trading software where charts and different instruments are shown. The chat then remains the focus and the trade list could focus the particular offer in the chat, while both remains open. Another option is to make the list a sidebar, like the channel info, but I think that might be worse since it couldn't be too wide.

Is there some design policy to not use separate windows within Bisq2? I could see it parting from the current feel but I believe there are benefits as well.

BTW, I really like the improved feel of the chat offers with colored buttons and "sell to" and "buy from" text.

@axpoems
Copy link
Contributor

axpoems commented Apr 22, 2024

@DrColver
Thank you for your inputs - I've been working on the mock-up and I've approached the issue as your second suggestion, the sidebar. While it's feasible, I've had to consider the space constraints from the begining. The overall is complete, but I'm still working on the details to give it a good experience.

There's no policy regarding seperate windows, but implementing it on smaller screens would worsen the user experience. Therefore, IMO, it's best to avoid that.

I appreciate your comments on the "sell and buy" issue, further improvements are still to come. The key focus was to adopt a user-centric perspective and maintain consistency throughout the application.

@DrColver DrColver closed this Apr 30, 2024
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.

3 participants