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 amendments list page #836

Merged
merged 20 commits into from
Nov 16, 2023
Merged

Conversation

pdp2121
Copy link
Collaborator

@pdp2121 pdp2121 commented Sep 25, 2023

High Level Overview of Change

The Explorer will roll out new features to include voting data (amendments + fee) soon. This page will be one of the new feature added.

Add a amendments page to display all amendments in voting and enabled.

Note that the links to specific amendment summary page (name, id) is not yet enabled (still white text). It will be updated once the amendment summary page has been created.

Please do not merge this PR until the validator-history-service has been updated with voting data.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Translation Updates
  • Release

Before / After

Desktop View

Screenshot 2023-10-12 at 2 53 18 PM

Mobile View

Screenshot 2023-09-25 at 5 24 08 PM

Add Amendment Summary page for each individual amendment

@pdp2121 pdp2121 changed the title feat: add amendments list page [DO NOT MERGE] feat: add amendments list page Sep 25, 2023
@mvadari
Copy link
Collaborator

mvadari commented Sep 25, 2023

What's the difference between "voters" and "consensus"?

@pdp2121
Copy link
Collaborator Author

pdp2121 commented Sep 25, 2023

What's the difference between "voters" and "consensus"?

Voters is the number of UNL voters who voted 'Yea', while consensus is voters/number of UNL validators. You can tell the consensus by voters and vice versa, I just follow the design here

@bugsbunnies
Copy link
Collaborator

bugsbunnies commented Sep 25, 2023

Can we move the Name column a bit closer to the Voters column? Right now it looks very close to ID and there's a lot of space in between Name and Voters. We can also increase the content area for the ID column as well by a few characters to balance it out

@pdp2121
Copy link
Collaborator Author

pdp2121 commented Sep 26, 2023

Can we move the Name column a bit closer to the Voters column? Right now it looks very close to ID and there's a lot of space in between Name and Voters. We can also increase the content area for the ID column as well by a few characters to balance it out

@bugsbunnies I have updated the distance between ID and Name (see the updated screenshot). We do not want to show a lot of ID column since it is insignificant and can be distracting

@pdp2121 pdp2121 mentioned this pull request Oct 4, 2023
1 task
@pdp2121 pdp2121 requested a review from jonathanlei October 6, 2023 20:44
@ckniffen
Copy link
Collaborator

Not really a fan of the navigation change. I think the "Network" navigation item should not be collapsed with "Explorer".

Instead i would like to see:

  • Explorer
  • Network
    • Nodes
    • Validators
    • Upgrade Status
    • Amendments
  • xrpl.org
  • Github

src/containers/Amendments/index.tsx Outdated Show resolved Hide resolved

it('renders all parts', () => {
const wrapper = createWrapper({ amendments })
expect(wrapper.find('tr').length).toBe(amendments.length + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see some more detailed tests that check specific column's data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The detailed test for each column (for both enabled and voting amendments) is in the amendments.test.js file. I'm just following the same structure as the tests in the Network folder here.

src/containers/shared/vhsTypes.ts Outdated Show resolved Hide resolved
src/containers/Amendments/index.tsx Outdated Show resolved Hide resolved
@pdp2121
Copy link
Collaborator Author

pdp2121 commented Oct 12, 2023

Not really a fan of the navigation change. I think the "Network" navigation item should not be collapsed with "Explorer".

Instead i would like to see:

  • Explorer

  • Network

    • Nodes
    • Validators
    • Upgrade Status
    • Amendments
  • xrpl.org

  • Github

@ckniffen the navigation has been updated as in the screenshot.

@pdp2121 pdp2121 requested a review from ckniffen October 13, 2023 15:18
@pdp2121 pdp2121 changed the title [DO NOT MERGE] feat: add amendments list page feat: add amendments list page Nov 15, 2023
}
}

.wrap {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try using <section> tags instead. I think there is styling for that already that falls within these sizes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the only place where the same style is being used in .network-page .wrap and I don't see any <section> tag being used anywhere else in the code

Copy link
Contributor

@jonathanlei jonathanlei left a comment

Choose a reason for hiding this comment

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

LGTM

@pdp2121 pdp2121 merged commit f712e7b into ripple:staging Nov 16, 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.

5 participants