-
Notifications
You must be signed in to change notification settings - Fork 1
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
5 clean the pendulumpay repo #13
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
06539b4
to
21ce977
Compare
21ce977
to
cf811ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up this repository.
I have two general questions:
-
Why are pages and components in this nested structure inside as
index.tsx
files? E.g., instead of (the more obvious?)pages/Landing.tsx
it ispages/landing/index.tsx
. I personally find the latter harder to work with because all open pages in my tabs are just calledindex.tsx
and navigation is harder. -
This is also something personal, but I like to keep the number of dependencies to a minimum, particularly for a project that deals with money. Npm is just ridden with transitive dependencies and each is a potential vulnerability. Furthermore, the built JS might balloon to too many megabytes – this was already a big burden for DTransfer.
As an example I seebig.js
,luxon
andlodash
. Each use case here in this template can be replaced by just a little bit more modern JavaScript. If we want to use a bigint library, then we should probably usebn.js
as this is already used in polkadot-js.
package.json
Outdated
"stellar-sdk": "^10.4.1", | ||
"ts-node": "^10.9.1", | ||
"yup": "^1.2.0" | ||
"ts-node": "^10.9.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should rather be a devDependency
package.json
Outdated
"react-hook-form": "^7.43.2", | ||
"react-router-dom": "^6.8.1", | ||
"react-table": "^7.8.0", | ||
"react-toastify": "^9.1.3", | ||
"stellar-sdk": "^10.4.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is most likely not required for pendulum-pay.
The idea here is to group all files that relate to one component or page into one directory. For example in the component case, imagine that we define the styles for that component in a stylesheet just for that component or we have a test, then they could be grouped with
Right, I'll change that. |
Thanks for that reasoning, almost thought so, but then thought that our components will most likely be self contained and the css will be in the ts file. So yeah, it might just boil down to whether we use tests or not. I'm fine either way then. |
I removed the unnecessary dependencies now but left the |
This removes a lot of the portal-related functions and components while keeping the basic structure alive.
I would not recommend looking at the changes displayed for this PR but rather having a look at the branch directly.
Notes
assets
folderAbout preact
When starting the development on the portal, our freelance frontend developer decided to set it up for use with preact.js. It apparently has some performance benefits over 'regular' react and it's very simple to use because it exposes the same functions/hooks with the same interfaces. So all we have to do going forward is to import hooks from
preact/compat
instead ofreact
.But we can also remove it if you have strong opinions on this @TorstenStueber.
Closes #5.