feat: add a unified formatting standard#545
feat: add a unified formatting standard#545Roaring30s wants to merge 16 commits intolivepeer:mainfrom
Conversation
|
@Roaring30s is attempting to deploy a commit to the Livepeer Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
@ECWireless Out of all the PRs I have, I recommend checking this one first since this touches everywhere and the likelihood of annoying conflicts emerging throughout time are very high here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
@Roaring30s I merged a merged conflict fix. Could you make this changes to fix it?
|
|
@ECWireless I have resolved the feedback from the AI review and fixed the conflicts. I also did a quick pass on dev to see if there were any visual discrepancies and it checks out. |
ECWireless
left a comment
ECWireless
left a comment
There was a problem hiding this comment.
This nearly looks good to me besides one minor comment (+ the VotingWidget comment`). Will merge on Monday if comments get addressed by then, unless @rickstaa has any feedback on it
| <Flex css={{ alignItems: "center", justifyContent: "space-between" }}> | ||
| <Text css={{ fontSize: "$2", color: "$neutral11" }}> | ||
| {abbreviateNumber(proposal.votes.total.voters, 4)} LPT voted ·{" "} | ||
| {formatNumber(proposal.votes.total.voters, { |
There was a problem hiding this comment.
Looks like this removes the LPT unit.
Before:
After:
| choiceId={1} | ||
| pollAddress={poll?.id} | ||
| > | ||
| Change Vote To No |
There was a problem hiding this comment.
Language here should be "against"
| choiceId={0} | ||
| pollAddress={poll?.id} | ||
| > | ||
| Change Vote To Yes |
There was a problem hiding this comment.
Language here should be "for" to be consistent with how voting results are displayed elsewhere
| size="4" | ||
| pollAddress={poll?.id} | ||
| > | ||
| Yes |
| choiceId={1} | ||
| pollAddress={poll?.id} | ||
| > | ||
| No |
There was a problem hiding this comment.
This should be "against"
| myAccount: AccountQuery; | ||
| }; | ||
|
|
||
| const Index = ({ data }: { data: Props }) => { |
There was a problem hiding this comment.
I'm actually confused about what this new file is for. We already have both a TreasuryVotingWidget and a PollVotingWidget
2 participants
Description
This PR standardizes the number and currency formatting across the Explorer, specifically targeting the Root, Treasury, Orchestrator, Account and Voting pages. It replaces ad-hoc manual formatting (e.g., hardcoded division
/ 10000, manual" LPT"string concatenation) with our centralized utility functions in@utils/numberFormatters.tsType of Change
Related Issue(s)
Fixes: #442
Changes Made
🏛 Treasury & Proposals
formatWeightwhich relies on formatLPT.🗳 Voting
🎻 Orchestrators
👤 Accounts
We have established the following "Big Rules" for how numbers are displayed:
1. LPT Amounts (formatLPT)
15K LPT,2.5M LPT) by default, unless specified otherwise (e.g., in detailed tables).< 0.01show as< 0.01 LPT.< 0.0001show as< 0.0001 LPT.0 LPT(never0.00 LPT).2. Voting Power (formatVotingPower)
12.50K LPT).3. Percentages (formatPercent)
12.34%).50%instead of50.00%) for cleaner UI strings, unless a fixed precision is forced.10000or1000000) with named constantsPERCENTAGE_PRECISION_TEN_THOUSANDandPERCENTAGE_PRECISION_MILLIONfrom@utils/web3.4. ETH Amounts (formatETH)
< 0.0001are explicitly shown as< 0.0001 ETH.1,234.5678 ETHinstead of1.23K ETH), as precise ETH values are often critical.5. USD Currency (formatUSD)
$prefix.$1,234.56).Testing
How to test (optional unless test is not trivial)
Youre going to need to make a coffee and sit down and just inspect all the numbers on the pages to ensure formatting meets the standard.
Impact / Risk
Risk level: Low
Impacted areas: UI
User impact: Better number visual
Rollback plan: PR revert
Screenshots / Recordings (if applicable)
None
Additional Notes
Warning to Reviewer
I don't have the livepeer com key so I couldn't visually test the charts found on root page. If you could eyeball that part thoroughly, that should give us 100% coverage.
I am also slowly ridding of excessive useMemo's that may get in the way of my PR's throughout the project. We are currently using them to memoize simple arithmetic. The overhead introduced by the memos are heavier than re-rendering simple arithmetic unless it causing a render cascade elsewhere which I am checking for.