-
Notifications
You must be signed in to change notification settings - Fork 41
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
Redo Caroussel #216
Redo Caroussel #216
Conversation
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.
Commented code needs to be removed of course. This should be refactored correctly.
The idea is to make separate UIs for separate capabilities. At this time we are not considering moving to React etc for this. |
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. |
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 👌 |
@@ -168,8 +170,15 @@ table[data-make-claim-rendered] tr#additional-details-border + tr > * { | |||
table #additionalDetailsTable { | |||
opacity: 0; | |||
pointer-events: none; | |||
transform: translate(-50%, -90px); | |||
transform: translate(-50%, -133px); |
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 doesn't look right.
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.
Humm let me look for a better way to do it then
#table-target table[data-details-visible="true"]:nth-child(even) #additionalDetailsTable { | ||
opacity: 1; | ||
pointer-events: none; | ||
transform: translate(-50%, -70px); |
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 works for more than two tables?
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.
Yes it does work for both
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.
For more than two tables. Like three or more.
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.
You're right works fine for 3 but upwards of that it breaks. I'm looking into it thanks
@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. |
That's a good observation and if that was closer to implementation, I would tend to agree. Regarding that particular task:
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. |
@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". |
Resolves #196
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.