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

feat: add amendment summary page #855

Merged
merged 30 commits into from
Nov 15, 2023
Merged

Conversation

pdp2121
Copy link
Collaborator

@pdp2121 pdp2121 commented Oct 3, 2023

High Level Overview of Change

The page would include:

  • Information about the amendment
  • List of voted Yes and No validators (for amendments in voting)
  • A graph showing distribution of votes broken down by UNL and non-UNL validators (for amendments in voting)

This PR would also include search by Amendment Id/Name

Type of Change

  • New feature (non-breaking change which adds functionality)

Before / After

Amendment in voting (Desktop)

localhost_3001_amendment_Clawback (1)

Amendment in voting (Mobile)

localhost_3001_amendment_Clawback(iPhone 12 Pro) (1)

Amendment enabled

Screenshot 2023-10-03 at 7 09 56 PM

Chart legend

Screenshot 2023-10-04 at 11 49 39 AM

Footnote

Screenshot 2023-10-04 at 4 36 09 PM

@bugsbunnies
Copy link
Collaborator

bugsbunnies commented Oct 3, 2023

@pdp2121 The Yeas and Nays colors are reversed, should be Yeas - Green, Nays - Magenta. In addition to the color fix, let's have the Yeas bar on the left and Nays on the right

@pdp2121
Copy link
Collaborator Author

pdp2121 commented Oct 3, 2023

@pdp2121 The Yeas and Nays colors are reversed, should be Yeas - Green, Nays - Magenta. In addition to the color fix, let's have the Yeas bar on the left and Nays on the right

@bugsbunnies Nice catch! I just updated the color and screenshots

@mvadari
Copy link
Collaborator

mvadari commented Oct 3, 2023

The "UNL"/"non-UNL" labels look like they're supposed to be labeling each of the bars. Can those titles be moved to the top of each graph?

@bugsbunnies
Copy link
Collaborator

bugsbunnies commented Oct 4, 2023

The "UNL"/"non-UNL" labels look like they're supposed to be labeling each of the bars. Can those titles be moved to the top of each graph?

I think the problem is that they're at an angle, can we just make it so they center align with each bar group and not at an angle?

image

@mvadari
Copy link
Collaborator

mvadari commented Oct 4, 2023

The "UNL"/"non-UNL" labels look like they're supposed to be labeling each of the bars. Can those titles be moved to the top of each graph?

I think the problem is that they're at an angle, can we just make it so they center align with each bar group and not at an angle?

Yes, agreed - I think it'd look better with a top label, but I'm not opposed to a bottom axis label.

@bugsbunnies
Copy link
Collaborator

The "UNL"/"non-UNL" labels look like they're supposed to be labeling each of the bars. Can those titles be moved to the top of each graph?

I think the problem is that they're at an angle, can we just make it so they center align with each bar group and not at an angle?

Yes, agreed - I think it'd look better with a top label, but I'm not opposed to a bottom axis label.

I suggested bottom label just to keep it consistent with the basics of a graph with y-axis label and x-axis label in their expected places. The tricky thing with a top label (if im imagining what you're saying correctly) is that if we give them a fixed position on the graph, they might not always be close to the data bars, and if we keep them close to the data, the label could move around depending on the bars height.

@mvadari
Copy link
Collaborator

mvadari commented Oct 4, 2023

Shouldn't the table only show UNL - or have tabs to show UNL vs non-UNL?

@pdp2121
Copy link
Collaborator Author

pdp2121 commented Oct 4, 2023

I have fixed the axis accordingly and updated the screenshots. The Simple of this page shows both Yeas(All), Nays(All), Yeas(UNL), and Nays(UNL) (The Yeas and Nays columns at the bottom also shows all validators with green checkmarks for UNL), while the amendments list page in #836 only count UNL for voters. I think we received quite a few requests on showing all validators instead of just UNL so I think it makes sense to show all in this summary page

@intelliot
Copy link
Collaborator

Agree that it's useful to show all validators (instead of just UNL) and it makes sense to show all.

dUNL validators should be marked with the green check.

optional - It would also be useful to have a footnote to indicate what the green check means: "this validator is on the vl.ripple.com UNL" (or vl.xrplf.org, whichever is accurate).

@mvadari
Copy link
Collaborator

mvadari commented Oct 4, 2023

IMO 1) it'd be useful to have a separate table that only shows the UNL (as well as one that shows all of them) and/or 2) the table should be sorted so that the UNL comes first (like we do in the validator tables).

@pdp2121
Copy link
Collaborator Author

pdp2121 commented Oct 4, 2023

Makes sense! I have moved the UNL validators to the top and added a footnote for clarification. I think this is clear enough to explain those columns.

src/containers/Amendment/index.tsx Outdated Show resolved Hide resolved
src/containers/Amendment/amendment.scss Outdated Show resolved Hide resolved
src/containers/Amendment/index.tsx Outdated Show resolved Hide resolved
src/containers/Amendment/index.tsx Outdated Show resolved Hide resolved
public/locales/en-US/translations.json Outdated Show resolved Hide resolved
@pdp2121 pdp2121 requested a review from ckniffen October 12, 2023 21:08
@pdp2121 pdp2121 changed the title [DO NOT MERGE] feat: add amendment summary page feat: add amendment summary page Nov 15, 2023
@pdp2121 pdp2121 merged commit a296037 into ripple:staging Nov 15, 2023
4 checks passed
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.

6 participants