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 shortcut list #3695

Merged
merged 8 commits into from
Dec 17, 2019
Merged

Conversation

chimp1984
Copy link
Contributor

Should be merged after #3694 (not hard requirement though)

Copy link
Contributor

@julianknutsen julianknutsen left a comment

Choose a reason for hiding this comment

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

NIT and questions

@chimp1984 chimp1984 changed the title Add short cut list Add shortcut list Nov 29, 2019
Copy link
Member

@freimair freimair left a comment

Choose a reason for hiding this comment

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

ahm I am not sure if I got your intentions but that is what caught my attention:

  • in AccountView pressing ... + n creates a new refund agent tab. Yet, the text says arbitrator.
  • there is no code to create a new arbitrator-tab, can we get rid of the remnants?

It seems there is some tangling up of "arbitrator" and "refund agent". We should clear that up to avoid confusion.

minor stuff:

  • Navigate to account and click: {0} shouldn't it be "press" instead of "click"

@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 6, 2019

It seems there is some tangling up of "arbitrator" and "refund agent". We should clear that up to avoid confusion.

Atm we used in all user facing views arbitrator instead of refund agent to reduce unnecessary concerns when we switched to the new trade protocol.
If someone tries to register an arbitrator (new refund agent) we labeled it refund agent to make it clearer to the new refund agents that they are in the correct registration view. So I don't think it is a good idea to change the label to arbitrator now to prevent confusion for the real refund agents.
I think maybe in a couple of releases when we get rid of most of the legacy arbitrator parts there might be the point also to rename the refund agents.

  • Navigate to account and click: {0} shouldn't it be "press" instead of "click"

NIT - Yes it would be good to be consistent in this view if we want to use click or press.

@chimp1984
Copy link
Contributor Author

@freimair

refund agent vs. arbitrator

We use refundAgent in the domain (code) but left arbitrator as the term for users (UI). There are some discussions for a better term, but so far arbitrator (UI) is refund agent and old arbitrator is "legacy arbitrator".

click

Will change it.

@freimair
Copy link
Member

freimair commented Dec 7, 2019

  • there is no code to create a new arbitrator-tab, can we get rid of the remnants?

what about dead code?

but so far arbitrator (UI) is refund agent and old arbitrator is "legacy arbitrator".

seems reasonable. however, why is the refund agent tab then titled "refund agend registration"?

@chimp1984
Copy link
Contributor Author

  • there is no code to create a new arbitrator-tab, can we get rid of the remnants?

what about dead code?

Removing the legacy arbitator domain is a bit of a bigger task. There might be some usage in messages, so simply deletion will not be enough.

but so far arbitrator (UI) is refund agent and old arbitrator is "legacy arbitrator".

seems reasonable. however, why is the refund agent tab then titled "refund agend registration"?

"Arbitrator" is for users. "refund agend registration" is for refund agents to make it more clear and to avoid confusion for the refund agent.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Dec 11, 2019

@freimair Can you review again?

@freimair
Copy link
Member

I am stepping back from reviewing this.

First, IMO, if we remove the short-cut to even trigger a "legacy arbitrator" tab, the code remnants should be removed. This is dead code. If we do not remove it right away, it might stay there until a newcomer asks about what that is.

Second, I do not like the dual vocabulary. It is only confusing.

@freimair freimair dismissed their stale review December 11, 2019 14:26

I am stepping back from reviewing this.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Dec 11, 2019

@freimair I only wanted to add a small feature as it is annoying to look up the shortcuts in the code each time one needs them. It was not intended to change existing naming schemes (refund agent vs. arbitrator) as there is anyway some discussion going on for a better term. As well it would be far beyond the scope of that PR to remove the domain code of legacy arbitration which carries also some risks and will require lots of testing with backward compatibility. As the risk/effort/benefit ration is pretty bad for that effort I also do not have intention to work on that any time soon. Too much other important stuff waiting to get implemented...

I understand that this discussion here becomes too much of an effort and I am ok with closing that PR as well. Not such an important feature to justify to burn so much time for reviewers and author.

@sqrrm sqrrm self-assigned this Dec 12, 2019
@sqrrm
Copy link
Member

sqrrm commented Dec 12, 2019

I will take a look at this as I want the short cut list added (too annoying looking through the code indeed)

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 on Regtest and everything is working as expected.

@ripcurlx ripcurlx merged commit 6f1dbf1 into bisq-network:master Dec 17, 2019
@chimp1984 chimp1984 deleted the add-short-cut-list branch December 22, 2019 18:35
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.

None yet

6 participants