search

ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)
https://pay.ubq.fi
8 stars 28 forks source link

Redo Caroussel #216

Closed jordan-ae closed 3 weeks ago

jordan-ae commented 3 months ago

Resolves #196

Screenshot 2024-03-31 231444

Still working on getting everything working properly. But so far I've gotten the cards to show in a stack rather than a carousel.

In my opinion the codebase is too rigid and thus isn't easily maintainable and scalable. For example the simplest fix will lead to extensive refactoring which makes the process of iterating and improving very difficult. I believe making it more modular will help us scale and maintain it better as we grow and I'd love to discuss the potential of this with you guys.

0x4007 commented 3 months ago

The idea is to make separate UIs for separate capabilities. At this time we are not considering moving to React etc for this.

ubiquibot-continuous-deploys[bot] commented 3 months ago
c5af240
277fd02
0x4007 commented 3 months ago

c5af240

Only showed one for me with the incorrect control buttons fyi

jordan-ae commented 3 months ago

c5af240

Only showed one for me with the incorrect control buttons fyi

Fixed only one card showing. Please can you check again? Abt the controls I'm working to modify the controls to work on each card. Since they were built to accommodate only one card being on the screen per time I have to rebuild the way the controls work, which is taking up. But will be done with that ASAP.

jordan-ae commented 3 months ago

The idea is to make separate UIs for separate capabilities. At this time we are not considering moving to React etc for this.

Okay, I understand. My remark isn't centered around what we are using. Vanilla TS works fine, but the implementation of what we're doing could be made better. As it stands the code is very rigid and small changes lead to the biggest refactor which is not the best for scalability and maintenance in my opinion.

0x4007 commented 3 months ago

The idea is to make separate UIs for separate capabilities. At this time we are not considering moving to React etc for this.

Okay, I understand. My remark isn't centered around what we are using. Vanilla TS works fine, but the implementation of what we're doing could be made better. As it stands the code is very rigid and small changes lead to the biggest refactor which is not the best for scalability and maintenance in my opinion.

I agree but React etc comes with its own set of problems. Given that this is a single view app, react etc is overkill and will get in the way more than it will help.

jordan-ae commented 3 months ago

The idea is to make separate UIs for separate capabilities. At this time we are not considering moving to React etc for this.

Okay, I understand. My remark isn't centered around what we are using. Vanilla TS works fine, but the implementation of what we're doing could be made better. As it stands the code is very rigid and small changes lead to the biggest refactor which is not the best for scalability and maintenance in my opinion.

I agree but React etc comes with its own set of problems. Given that this is a single view app, react etc is overkill and will get in the way more than it will help.

Okay got it 👌

jordan-ae commented 2 months ago

@pavlovcik Have a little curiosity. Came across plans for payouts V2 #174. I'm not sure what timeline is planned for V2, but since this is in view already, won't it be better to hold off on changing the nature of the permit display? As this will all be discarded in a couple weeks or months. Just asking cause it might save us some cost and development time on task that might not be of the highest priority right now viewing the works in plan.

0x4007 commented 2 months ago

Came across plans for payouts V2 https://github.com/ubiquity/pay.ubq.fi/issues/174. I'm not sure what timeline is planned for V2, but since this is in view already, won't it be better to hold off on changing the nature of the permit display?

That's a good observation and if that was closer to implementation, I would tend to agree.

Regarding that particular task:

  1. There's several dependencies of it that aren't ready, and those are separate projects that haven't been started on yet. This primarily includes everything card related, data storage related (we are migrating databases) and the new UI itself.
  2. Delays happen normally, so I wouldn't be so optimistic on timelines for this
  3. The task specification isn't even finalized, hence why there is no pricing information yet.

Given those points, I think we are optimistically a few months out until implementation. I think we would be able to make use of multiple permit claims code for awhile, and its possible it could even be included in the new specification.

rndquu commented 1 month ago

@jordan-ae Is it ready for review? If that's the case then pls fix code conflicts and mark the PR as "ready for review".