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

refactor: migrate numeral to numbro #237

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

rickstaa
Copy link
Contributor

@rickstaa rickstaa commented Dec 3, 2023

This pull request deprecates the use of the unmaintained numeral-js library and replaces it with the numbro. This was done to fix #236 and ensure the explorer is future-proof.

Copy link

vercel bot commented Dec 3, 2023

@rickstaa is attempting to deploy a commit to the Livepeer Team on Vercel.

A member of the Team first needs to authorize it.

@rickstaa rickstaa marked this pull request as draft December 3, 2023 14:26
@rickstaa
Copy link
Contributor Author

rickstaa commented Dec 3, 2023

I conducted a swift review to ensure the proper display of all Explorer formats, addressing changes resulting from recent library updates. I corrected the discrepancies I identified but encountered two outstanding tasks:

  • Overview Widget and API Key: I couldn't edit the .env file to include the correct API key for the overview widget. This requires attention to ensure its functionality.

  • Testing Wallet Display: The widgets are shown when a wallet contains LPT and stakes or un-stakes at an orchestrator needs verification. Unfortunately, the absence of a CONTRIBUTING.md in this repository has left me seeking guidance on the optimal testing approach for these changes. I attempted to find a Goerli LPT test faucet but came up empty 🤔.

I would appreciate any insights or guidance you can give me on testing procedures 🙂.

@rickstaa rickstaa force-pushed the migrate_numeral_to_numbro branch 4 times, most recently from 97cd507 to 34f579b Compare December 3, 2023 15:11
This commit deprecates the use of the unmaintained
[numeral-js](https://github.com/adamwdraper/Numeral-js) library and
replaces it with the [numbro](https://github.com/BenjaminVanRyseghem/numbro).
This was done to fix livepeer#236 and ensure the explorer is future-proof.
@rickstaa rickstaa marked this pull request as ready for review December 10, 2023 16:17
Copy link

vercel bot commented Dec 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-arbitrum-goerli ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2023 5:59pm
explorer-arbitrum-one ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2023 5:59pm

Copy link
Contributor

@0xcadams 0xcadams left a comment

Choose a reason for hiding this comment

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

The build is failing - this may fix it

components/OrchestratorList/index.tsx Outdated Show resolved Hide resolved
@0xcadams
Copy link
Contributor

Thanks for this! Some changes we can hopefully make here

Co-authored-by: Chase Adams <[email protected]>
@0xcadams 0xcadams merged commit 8b2c557 into livepeer:main Dec 12, 2023
3 checks passed
0xcadams added a commit that referenced this pull request Dec 12, 2023
@0xcadams
Copy link
Contributor

Hey @rickstaa it looks like this broke the explorer, I had to revert
Screenshot 2023-12-12 at 11 15 13 AM

@rickstaa
Copy link
Contributor Author

rickstaa commented Dec 12, 2023

Hi @0xcadams,

No problem at all. As mentioned in this comment, I apologize for not thoroughly testing all parts of the explorer before submitting it. In hindsight, I should have kept it as a draft. I understand the inconvenience it caused 😅.

I'd happily create a new pull request to test all changes thoroughly. However, I could use some guidance on the best testing practices for all my changes. Specifically, there are two areas of the explorer that I couldn't test:

  1. Overview Widget: as you've noticed, this widget is currently broken in your screenshot. I couldn't configure the .env file with the correct API key to make the overview widget function properly.

  2. Testing Wallet Display: The widgets appear when a wallet contains LPT and stakes or unstakes at an orchestrator, and this part of the code needs verification. Unfortunately, the absence of a CONTRIBUTING.md file in this repository has left me uncertain about the best approach for testing these changes. I attempted to find a Goerli LPT test faucet but could not locate one 🤔.

Your guidance on how to effectively test these changes would be greatly appreciated.

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.

Delegated stake NaN display bug
2 participants