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.

Sign up for GitHub

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

Jump to bottom

Faucet: Add handling for chains API #685

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Faucet: Add handling for chains API #685

wants to merge 1 commit into from

Conversation

givingmittal
Copy link

Closes #534

mateuszjasiuk
mateuszjasiuk requested changes Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for the contribution. I've left two comments. Also be sure that your branch is rebased with current main and that you have correct linting.
You can run yarn prepare in the root of the monorepo to setup husky.

apps/faucet/src/utils/chainsApi.ts
@@ -0,0 +1,66 @@
import {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you just reuse the current api.ts ?

apps/faucet/src/App/App.tsx

// Fetch settings from faucet API
(async () => {
try {
let startsAtToMilliseconds = startsAt * 1000;
let startsAtText = defaults.startsAtText
try{
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to move this try...catch block to the separete function. I think that would make code more readable. Also it should help with getting rid off mutations in multiple places. At the moment it's a bit difficult to follow what is going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszjasiuk mateuszjasiuk mateuszjasiuk requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Faucet: Add handling for chains API
2 participants
@givingmittal @mateuszjasiuk