Slider refactor #193
Slider refactor #193
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
mcgingras
force-pushed
the
slider-refactor
branch
from
3b4ae78
to
2f8a917
Compare
| successCallback={() => { | ||
| closeSlider() | ||
| successToast({ | ||
| message: "Email notification settings updated", | ||
| }) | ||
| }} |
There was a problem hiding this comment.
I kind of like this success callback pattern rather than handling the closing in the component
There was a problem hiding this comment.
I like that having the successCallback seems to abstract away some seemingly common patterns of showing a toast upon closing the modal, but I think if we continue to follow this pattern, more of the components' business logic would need to be put into this manager. For instance we might need to add an errorCallback or include more data-specific messaging within the toast. Since we need to call the closeSlider function anyways within each component, it might be easier to define and handle the respective successCallbacks within each content component as well.
There was a problem hiding this comment.
I think the original design was the fact that components like sendTokensContent are sometimes put in right sliders (on desktop) and sometimes they are their own pages. So the way you handle a successful completion depends on the context of where you are using the component. In the case of the slider, you want to clear the slider and maybe add a toast. In the case of a page, you might want to redirect to a different page.
The design with successCallback was to make it a bit easier to scope what sort of cleanup action you wanted to the calling component. In this case above, that would be the slider manager controlling the cleanup when the sendToken content is used within a slider. The sendTokens page (on mobile) would be able to declare a callback that might add a redirect.
This way, the sendTokensContent doesn't have to care or be aware of the many different places it could be used, each possible having a different approach for cleaning it up. The other way to do it would be storing all the logic in the component, then possibly passing a prop down to specify it's "type" like type="slider | page | modal | ..." then in the component we could do a bunch of ifs to check what type it is and handle the cleanup within the component.
Thats why I was a fan of moving the logic for successCallback out of the component itself
There was a problem hiding this comment.
Thanks for the explanation! Rip to branching logic. I think I still prefer handling all the logic within the component itself or use your other suggestion of passing the prop for slider/page/modal (could be useful for instrumentation purposes too). I think it's easier for me to see all that logic being handled in one place instead of split between the slider manager and the component, but it's for sure not as pretty to look at especially with all of the lines of code that already exist within the content components. I think I'm mainly worried about a future use case where we'd need to handle business logic in a success callback where we'd see a weird divide of partially handling the success here and the app and also the fact that we might need to handle mobile use cases differently therefore the conditional logic of handling mobile vs. slider logic might still exist within the component 🥲
|
Doing a visual qa first! Found this mismatch of the footer with the buttons where the execute and reject button aren't fully on the page on this branch. Also I see what you mean on the ux of the request details slider - it's kinda weird to see an initial loading screen of the request details. staging: preview url: |
|
on mobile, both the slider and the bottom sheet appear
|
|
I'm not sure why, but when I come to the empty states here on the automations page and click "Create", visit the automations page, and then try to go back, I can't for some reason, but this works on staging 🤔 I wonder if it has something to do with shallow routing
|
|
Hmm, looks like we need to be careful about grabbing addresses from the query param. Right now, I can open "claim" drawers for other people. I'm unable to claim because the contract breaks, but it feels like an exploit 😈
|
| chainNameAndSafeAddress: string | undefined | ||
| } | ||
| if (address) { | ||
| recipientAddressParam = address as string |
There was a problem hiding this comment.
maybe the recipient address should always be the logged-in user?
There was a problem hiding this comment.
ya, if its a profile page we could use the logged in user, and if its a terminal page then it should belong to the terminal but we should definitely make sure the person claiming is authed.
I agree it feels like an exploit, but "claiming" doesn't mean you are sending the tokens to yourself. If I were able to see the claim CTAs on your profile, all I would be able to do is pay the gas to execute the transaction that sends the tokens to your address lol. It's a reverse exploit, exploiting the exploiter to pay the gas for you! 😆
but I agree, just to make the app not feel janky we should make sure to protect these pages.
|
I think |
mcgingras
force-pushed
the
slider-refactor
branch
from
1e07d4e
to
d55af0a
Compare
Experimental devx improvements for managing state of sliders.
So far I'm the least convinced devx is improved when the state involves a given id. For example, the request details slider requires knowledge of the state of the "active request". Before, this was easier because the slider was rendered in the context of the requestsList, so it had knowledge of all of the requests. However, if we isolate the slider into it's own component, the slider no longer has knowledge of any requests, and there is no trivial way to pass down the entire request object. The best way I've found is using URL query params to store the requestId, then rehydrating the full request within the content of the slider for the request details page. However, this is a bit of a pain, because we have to fetch the request object again, even if we already have it on certain pages. I'm not sure if the devx feels much better either, although I'm trying.
Aside from that, I feel like the devx is a great improvement. I love having all of the sider content in one component, and I love being able to manage it with a consistent interface and suite of actions.
Still a work in progress.
bugs