-
Notifications
You must be signed in to change notification settings - Fork 126
Refactor/streams take 2 #1225
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
base: staging
Are you sure you want to change the base?
Refactor/streams take 2 #1225
Conversation
# Conflicts: # src/containers/Ledgers/LedgerMetrics.jsx # src/containers/Ledgers/Ledgers.jsx
# Conflicts: # src/containers/Network/Hexagons.jsx
ckeshava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are notes for myself -- I'd like to address/work on these points before merging this PR.
| transactions: any[] | ||
| index: number | ||
| hashes: LedgerHash[] | ||
| seen: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Ledger xrpl.js type already contains some of these fields, except for seen. It is worthwhile to see if we can re-use that type. Here is the source file.
| &.tooltip-paystring { | ||
| width: 274px; | ||
| font-size: 12px; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paystring project has been deprecated for a few years. This css section can be removed.
| import PayStringLogomark from '../../shared/images/PayString_Logomark.png' | ||
| import QuestIcon from '../../shared/images/hover_question.svg' | ||
| import Tooltip from '../../shared/components/Tooltip' | ||
| import { Tooltip } from '../../shared/components/Tooltip/Tooltip' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PayString related files are not used anymore in the Explorer code base.
| function updateMetrics() { | ||
| const ledgerChain = Object.values(ledgers) | ||
| .sort((a, b) => a.index - b.index) | ||
| .slice(-100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The tooltip on the Explorer website mentions the last 50 ledgers being used for these metrics. This value needs to be updated.
| import successIcon from '../../images/success.png' | ||
| import { localizeDate } from '../../utils' | ||
| import '../../css/tooltip.scss' | ||
| import PayStringToolTip from '../../images/paystring_tooltip.svg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Paystring portion of the code needs to be deleted.
| )} | ||
| <RouteLink | ||
| to={VALIDATOR_ROUTE} | ||
| params={{ identifier: selectedValidator }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

High Level Overview of Change
This is the work of @ckniffen. Many thanks. I'd like to open a PR to understand/discuss it better.
Github doesn't let me comment/review files well without a PR. The PR description will be updated in a few days.
Context of Change
Type of Change
Codebase Modernization
Before / After
Test Plan