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

[UI] wormhole page #425

Closed
wants to merge 19 commits into from
Closed

[UI] wormhole page #425

wants to merge 19 commits into from

Conversation

cyphertrace
Copy link
Contributor

@cyphertrace cyphertrace commented Oct 10, 2022

Sorry for ugly code, it was bunch of experimentation and I'm refactoring in better mutations in consecutive PR.

Notion ticket: Ticket

Checklist

  • Github project and label have been assigned
  • Tests have been added or are unnecessary
  • Docs have been updated or are unnecessary
  • Preview deployment works (visit the Cloudflare preview URL)
  • Manual testing in #product-testing completed or unnecessary
  • New i18n strings do not contain any security vulnerabilities (e.g. should not allow translator to update email/url)
  • When fetching new translations from external sources, do a sanity check (e.g. get people who speak the language to check, shove all new translations into Google translate)

@cyphertrace cyphertrace added the feature New feature or request label Oct 10, 2022
@cyphertrace cyphertrace self-assigned this Oct 10, 2022
@github-actions
Copy link

github-actions bot commented Oct 10, 2022

✨ Deployment complete! Take a peek over at https://742f81f9.ui-storybook.pages.dev

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 10, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: f8a0655
Status: ✅  Deploy successful!
Preview URL: https://2b1a875c.swim-ui.pages.dev
Branch Preview URL: https://ui-wormhole-page.swim-ui.pages.dev

View logs

@cyphertrace cyphertrace marked this pull request as ready for review October 12, 2022 14:01
@nicomiicro
Copy link
Contributor

@cyphertrace I think there is an issue with the list container, I cannot scroll to the last element.

Screenshot 2022-10-13 at 12 26 00 PM

Copy link
Contributor

@nicomiicro nicomiicro left a comment

Choose a reason for hiding this comment

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

Nice work, I didn't test it, I wasn't sure if it's ready for review tbh

readonly status: TransferStatus;
};
// eslint-disable-next-line functional/prefer-readonly-type
let cacheTransferResults: TransferResult[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we store this to a normal react state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a hacky version, I will move all data to store and mutations, it will be more reactive.
The thing is that state wasn't fast enough to update with changes, previous state was always out of date, so I set the small caching to keep the track. But I'm changing that in next PR

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see. This usually goes away if you use the function form of the updater setState(prev => prev.concat([newTransferInfo]) (something like that) instead of setState(previous.concat([newTransferInfo]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried something similar (not with concat), but somehow prev wasn't good either, it didn't trig re-render. Maybe I should test with .concat. Thanks for tips!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, with previous and concat, still the same issue with stale data, so I got the repetition
Screenshot from 2022-10-13 14-14-47
Hopefully introducing store will make it better...

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't run the code, but I guess it is because the reference of transferInfo is not updated to latest yet when you running the function, so when you try to get, it does not exist (using old reference to transferInfo), and thus you append again to the state which lead to duplication. I think a less hacky way to fix that could be using useRef to get the latest reference of transferInfo.

readonly sourceEcosystemId: EcosystemId;
readonly targetEcosystemId: EcosystemId;
readonly amount: string;
readonly getTransferInfo: (
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: setTransferInfo or addTransferInfo would probably make more sense for something that returns void?

readonly handleTransfer: (data: TransferData) => void;
}

export const useTransfer = (): TransferResult => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: you might want to refactor this to use react-query mutation? I think you mentioned it somewhere already though so if it will be done in the next iteration feel free to ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, working on that :)

// eslint-disable-next-line functional/immutable-data
formattedTokens.push(wrappedToken);
}
return formattedTokens;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: you can also do this with a map so you avoid the disabling of the lint rule

return [token].concat(token.wrappedDetails.map(...))

@wormat
Copy link
Collaborator

wormat commented Oct 13, 2022

Closing in favour of #439

@wormat wormat closed this Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants