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

Add support/help links on the UI #5985

Merged
merged 13 commits into from Feb 9, 2022
Merged

Add support/help links on the UI #5985

merged 13 commits into from Feb 9, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 20, 2022

Fixes #5972


Screenshots:

s3
s1
s2
screen4
screen5
screen6

@ghost ghost marked this pull request as ready for review January 27, 2022 15:27
@ripcurlx
Copy link
Contributor

@xyzmaker123 Could you please adapt the links to point to matrix instead? See #5972 (comment)

@ghost
Copy link
Author

ghost commented Jan 28, 2022

@ripcurlx Done

ripcurlx
ripcurlx previously approved these changes Feb 1, 2022
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK - Tested it on Regtest

I did make some changes in the UI to make the question icon less prominent and improve readability of the informational text to make it not too hard to read.

Bildschirmfoto 2022-02-01 um 13 30 04
Bildschirmfoto 2022-02-01 um 13 41 26

I'm still waiting for @m52go to review the text with the here link I'm not too much a fan of as you don't know what to expect when clicking on here.

desktop/src/main/java/bisq/desktop/theme-dark.css Outdated Show resolved Hide resolved
Comment on lines 950 to 952
portfolio.pending.support.text.getHelp=Take a look at the trade process \
[here](https://docs.bisq.network/getting-started.html#4-send-payment)\
\n\nIf you have any problems you can try to contact the trade peer in the trade chat.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m52go Could you please review this paragraph? Thanks!

@ripcurlx ripcurlx added this to the v1.8.3 milestone Feb 1, 2022
@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 1, 2022

🤔 After reading through #5972 I'm having second thoughts on my changes above. Maybe we shouldn't use opacity in that case to make it a little bit more different to the help overlays in the columns.

@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 1, 2022

Maybe we should also add the link icon to the text links we use everywhere for text that links to an external website.

Copy link
Contributor

@m52go m52go left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary aims of my suggested changes:

  • Re-order text blocks to (1) encourage trader chat, (2) seek help on Matrix, and (3) read docs
  • Replace docs link with wiki links
  • Link to trade rules and dispute resolution instead of instructions on how to make a payment

Comment on lines 950 to 952
portfolio.pending.support.text.getHelp=Take a look at the trade process \
[here](https://docs.bisq.network/getting-started.html#4-send-payment)\
\n\nIf you have any problems you can try to contact the trade peer in the trade chat.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
portfolio.pending.support.text.getHelp=Take a look at the trade process \
[here](https://docs.bisq.network/getting-started.html#4-send-payment)\
\n\nIf you have any problems you can try to contact the trade peer in the trade chat.
portfolio.pending.support.text.getHelp=Many issues can be resolved by simply chatting with your trading peer.

It would be great if there was an empty line under the "Need help?" heading.

Comment on lines 926 to 927
portfolio.pending.stillNotResolved=If your issue still is not resolved ask the Bisq community or request support \
from a mediator using the link below: \n[https://bisq.chat](https://bisq.chat)
Copy link
Contributor

@m52go m52go Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
portfolio.pending.stillNotResolved=If your issue still is not resolved ask the Bisq community or request support \
from a mediator using the link below: \n[https://bisq.chat](https://bisq.chat)
portfolio.pending.stillNotResolved=If your issue remains unsolved, you can request \
support from support agents, mediators, and the Bisq community in our \
[Matrix chatroom](https://bisq.chat). \
\n\nFor reference, make sure to check \
out Bisq's [trading rules](https://bisq.wiki/Trading_rules) and \
[dispute resolution](https://bisq.wiki/Dispute_resolution).

@@ -108,7 +108,6 @@ private void buildViews() {
SimpleMarkdownLabel label = addSimpleMarkdownLabel(leftGridPane, ++leftGridPaneRowIndex);
AutoTooltipButton button = (AutoTooltipButton) addButtonAfterGroup(leftGridPane, ++leftGridPaneRowIndex, "");
SimpleMarkdownLabel footerLabel = addSimpleMarkdownLabel(leftGridPane, ++leftGridPaneRowIndex, Res.get("portfolio.pending.stillNotResolved"), 10);
footerLabel.getStyleClass().add("small-text");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid there is not enough space for making this text bigger. This change cause that there is no space between Need help? heading and paragraph (#5985 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using converted 11px instead of 10px is fine for the sub text IMO.

Bildschirmfoto 2022-02-09 um 10 20 22
Bildschirmfoto 2022-02-09 um 10 20 27

But it is still not enough for all use cases as you can see in the screenshots. So I'd just increase the SubView min height in that case.
Bildschirmfoto 2022-02-09 um 10 44 40
Bildschirmfoto 2022-02-09 um 10 44 37

@ghost
Copy link
Author

ghost commented Feb 4, 2022

@ripcurlx I see a few conversations are open on this pull request. Let me know if you'd like something from me regarding them.

Copy link
Contributor

@m52go m52go left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I usually test changes like this but my local environment was broken until just now.

Disregard my last review...suggestions in this new review should look like this. I don't see a use for the getHelp string.

Screenshot from 2022-02-04 12-50-41

core/src/main/resources/i18n/displayStrings.properties Outdated Show resolved Hide resolved
core/src/main/resources/i18n/displayStrings.properties Outdated Show resolved Hide resolved
@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 7, 2022

@ripcurlx I see a few conversations are open on this pull request. Let me know if you'd like something from me regarding them.

Could you please apply the last remarks from @m52go ? Thanks! I'll review it afterwards again and have a look at the ? icon if I find something that is consistent with our remaining UI, but still different enough to the ? roll over info in the columns.

Thanks!

@ghost
Copy link
Author

ghost commented Feb 7, 2022

@m52go What about other states? They doesn't looks good at the moment, f.e.:

case1
case2

@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 9, 2022

@m52go What about other states? They doesn't looks good at the moment, f.e.:

case1 case2

I'll have a look if we could slightly reduce the font size for the sub-text (something between regular and small-text) so we gain more space, but still can read it. I'll have a look at this PR today.

@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 9, 2022

Ok - I did some clean ups and adaptions to fix the text issue in pending trades and use the action color and a different icon for help links

Bildschirmfoto 2022-02-09 um 11 12 48
Bildschirmfoto 2022-02-09 um 11 12 44

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@ripcurlx ripcurlx merged commit 15abe71 into bisq-network:master Feb 9, 2022
@ghost ghost mentioned this pull request Feb 15, 2022
@ghost
Copy link

ghost commented Feb 19, 2022

Bug: text formatting under Check Payment

image

Also, that odd formatted line hyperlinks to e.g. bank that does not make sense:

image

@pazza83
Copy link

pazza83 commented Mar 12, 2022

Tested this and it works well

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.

Add Support/help links on the UI
3 participants