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

Batch Improvement - #1661

Open

GrandSchtroumpf opened this issue Feb 5, 2025 · 0 comments

Assignees

Collaborator

GrandSchtroumpf commented Feb 5, 2025 •

edited

Loading

Some dev improvement on the batch creation. Not blocking for the feature, so we didn't implemented them in the PR.

https://github.com/bancorprotocol/carbon-app/pull/1629/files/bb90cdd649fd1d84496118bfc1e1117e806123df#diff-8335c0c6f2712b74a65c32a006dcaa08d679066f902a7c637111ab571daa79b8
nitpick but I am not a fan of calling window.addEventListener('storage', handler); every place where you care about whether the user changed - and it's already in two places here. We can consider alternatives. A Custom hook will allow reuse and help sticking with a single event handler across the system. Maybe there's a better idea.

https://github.com/bancorprotocol/carbon-app/pull/1629/files/bb90cdd649fd1d84496118bfc1e1117e806123df#diff-8335c0c6f2712b74a65c32a006dcaa08d679066f902a7c637111ab571daa79b8
each function here has a few explicit responsibilities. They manage state, they read and write from local storage, they are aware of the DOM in order to define selectors and they define animations. This is breaking a few coding best practices.

GrandSchtroumpf assigned GrandSchtroumpf and zavelevsky

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