Closed martinklepsch closed 6 years ago
~PR is currently broken.~ Fixed. Found an issue where winner_login
was set to andytudhope
but there was no claim with that username. For now I'll just remove all PRs that have andytudhope
as winner from the UI. This should be addressed with some proper backend flag for revocation as soon as possible (see #284).
@churik when you get to test this, please check that the information in the dashboard makes sense. Outside that dashboard not much should have changed. And please ping @EugeOrtiz after it has been merged so she can check for necessary UI tweaks on staging.
Dashboard
It becomes even more inconvenient with a new dashboard because to see new data everywhere you should update it manually.
This is already entered for old dashboard in https://github.com/status-im/open-bounty/issues/361, but for now, it is spread to all data - new claims, when claim changes status, when it appears in To confirm payment.
EDIT Martin: ✅ (bounties are now reloaded in the same interval as other data)
VERIFY Chu: ✅
Confirm payout
button when bounty has several claims and one was mergedRequirements: GH account is whitelisted, signed app, test application is added to repo;
Submitted a claim via PR "B"; No confirm button;
Submitted a claim via PR "A"; can confirm payout;
So, as a result we have:
in GitHub: 1) closed issue with winner set to PR "A" owner (in SOB bot comment); state Pending maintainer confirmation 2) PR "A" - merged 3) PR "B" - opened
in SOB (Dashboard -> TO CONFIRM PAYMENT): 1) No confirm button 2) Submitted a claim via PR "B"
An example in testing env
:
EDIT Martin: ✅(Issue was that we found the winning claim based on a username match but in the case of multiple PRs we just took the first one. Changed it now so we exclude PRs that have not been merged.) VERIFY Chu: ✅
Requirements: GH account is whitelisted, signed app, test application is added to repo;
no overlap
EDIT Martin: ✅ VERIFY Chu: ✅
Other questions/improvements:
Merged bounties
- what is it for?I have no idea, what should be displayed here. Seems like it is for claims, which are already merged but not paid.
Is it for state Update ETH address
(when BHunter should set his ETH address)?
In any case, it is not clear what is the idea of this column and when bounties should appear there.
EDIT Martin: ✅ removed VERIFY Chu: ✅
For now when I reload the page (in Dashboard
), firstly "empty state" page is displayed, then spinner, then actual data is loaded.
Video: http://take.ms/pPozF
Would be nice to display spinner, then load actual data (remove this "empty state" page).
EDIT Martin: ✅
Any links on SOB are opened in the same tab. Actually to leave site is really easy, but to get back, you should type URL in the browser again. Would be nice to open links in new tabs.
EDIT Martin: ✅ removed VERIFY Chu: ✅
Steps: Requirements: GH account is whitelisted, signed app, test application is added to repo;
no overlapping
EDIT Martin: ✅ removed VERIFY Chu: ✅
EDIT Martin: ⚠️I think I fixed Scenario 2
but Scenario 1
is a bit tricky still because we don't track the issue state in our DB I think. Maybe @siphiuel has some idea on that. Question also arises how we would show these bounties then — are they "revoked"?
VERIFY chu:
scenario 2 is fixed now
Requirements: GH account is whitelisted, signed app, test application is added to repo;
Scenario 1: an issue is still in "Bounties not claimed yet." Scenario 2: PR is still in "TO MERGE" section
Scenario 1: issue disappear from "Bounties not claimed yet." Scenario 2: PR disappear from "TO MERGE" section
- SOB doesn't open a new tab when you click on the link (the same tab is used)
Would like to hear what @EugeOrtiz thinks about this. I'd assume most of our users know how to open stuff in a new tab and I think sometimes people get annoyed by apps making that decision for them.
develop
and in PRBefore - "Activity" page in current develop
, after - in current PR.
BTW, design of "Bounties" page is the same.
WEB DESKTOP General
Blue info “First time” message 🖖
Summary
Tabs
ToMerge cards
18px
is not part of type scale. E Let's try it with 1.25rem thenToConfirm cards
18px
is not part of type scale. E Let's try it with 1.25rem thenSecondary cards
WEB MOBILE
Case with ANT and SNT > ANT should get ETH colors. No matter the currency, if there are 2, they should get different styles.
I'm not sure I understand. Currently, ETH gets a particular color, and ERC20 tokens get the same.
From @churik: agree with @martinklepsch. Also don't fully understand why this happened:
Behavior > “Show more” should open batches of 6 cards and keep saying “Show more” if more cards are available, otherwise it should say “Show less”.
I started implementing this but then reverted my changes when stuff got hairy. I think for a V1 we should be okay with a show all button, especially since Andy is the only person with a large number of bounties. And even for him being able to expand all and then use browser tools like ctrl-f
might be better.
Border radii scale
We briefly touched on this yesterday. I'd like us to define a scale for border-radius
. Currently we have 4px, 8px, 16px but the designs use 8px, 10px. In a small refactoring I also made sure we use the same code to render balance-badges everywhere so we can easily change this across the entire site.
Requirements: GH account is whitelisted, signed app, test application is added to repo; there are some bounties in 'TO MERGE' column
the latest bounty on the top EDIT Martin: ✅ VERIFY chu: ✅
From @churik: agree with @martinklepsch. Also don't fully understand why this happened
Ok on the different styles (eth vs rest of tokens). @churik they have different styles from the primary cards because the ones at the bottom are secondary information and they take a lot of visual hierarchy with the badges background.
More UI feedback (posted here as requested):
@churik Other cases we should test:
For the summary view "Paid out...", "Open for..." what format are we using for the $ values? Until we do localization or agree on a standard approach, we should make sure they are consistent across the platform, which is e.g $4301.01 for now
@arash009 you can see on screenshot below (answer from @churik):
@arash009 we format numbers based on the users locale.
EDIT we format Dollar values based on the users locale. For crypto amounts we use the basic float format, eg 1000.25
Requirements: GH account is whitelisted;
TO CONFIRM
, TO MERGE
Error rendering component (in page > sW)
(anonymous) @ app.js?v=a65ebf0:782
app.js?v=a65ebf0:150 Uncaught Error: function Rf(a){switch(arguments.length){case 1:return Rf.j(arguments[0]);case 2:return Rf.h(arguments[0],arguments[1]);default:for(var c=[],d=arguments.length,e=0;;)if(e<d)c.push(arguments[e]),e+=1;else break;return Rf.l(arguments[0],arguments[1],new w(c.slice(2),0,null))}} is not ISeqable
at K (app.js?v=a65ebf0:150)
at Df (app.js?v=a65ebf0:201)
at Gf (app.js?v=a65ebf0:202)
at app.js?v=a65ebf0:1347
at Object.reagentRender (app.js?v=a65ebf0:1347)
at app.js?v=a65ebf0:780
at FH (app.js?v=a65ebf0:781)
at app.js?v=a65ebf0:782
at YG (app.js?v=a65ebf0:757)
at ZG (app.js?v=a65ebf0:758)
Unable to open TO MERGE
No errors;
TO MERGE
is not available;
EDIT Martin: ✅ VERIFY chu: ✅
Fix #218 Fix #361 Fix #338
Stuff left to do