feat: add push to all button to UI #2969
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| padding: theme.spacing(0), | ||
| }, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
I was thinking and if we want to make it perfect maybe we shouldn't remove the padding from the lis. It looks very good overall but the hover effect on the items looks a bit weird. Maybe we should remove some of these specifics, clean some of this up, and try to only style a div for the separator and button below, but I'm not sure.
Something we can run by @NicolaeUnleash and see what he thinks.
There was a problem hiding this comment.
I think we can iterate on the details later, so we can have this ready to be shipped with 4.20. Also, maybe we should have a .styles.ts file as specified here?
There was a problem hiding this comment.
Yeah, good point.
We are actually moving away from .styles.ts files and preferring styled components like you're doing here instead, so this looks good as is.
...ureView/FeatureVariants/FeatureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx
Show resolved
Hide resolved
| const [pushToAnchorEl, setPushToAnchorEl] = useState<null | HTMLElement>( | ||
| null | ||
| ); | ||
| const pushToOpen = Boolean(pushToAnchorEl); |
There was a problem hiding this comment.
In terms of code organisation since this is a computation based on a state, maybe it belongs some lines below instead, but it's a tiny nitpick based on preference.
There was a problem hiding this comment.
It was some lines below, but I moved it up to keep the related components together... otherwise, you may think you need to use pushToAnchorEl for reading the state and never realize that there's a pushToOpen that should be used instead... which is a bit confusing IMO but 🤷
| } catch (e) { | ||
| throw e; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I know that this is copy pasted from the other methods, and we always do it like this, me included. Not sure about the original reasoning but the fact is that this could optionally be simplified.
There was a problem hiding this comment.
I believe the try catch throwing an exception without doing anything is pointless... I could just replace:
try {
const res = await makeRequest(req.caller, req.id);
return res;
} catch (e) {
throw e;
}with
return makeRequest(req.caller, req.id);and now there's no need to await because it's the return statement.
But it's specified that way in this ADR. Maybe the intent of the ADR was to say that you should do something with the exception? Or perhaps it's something we carried away from another place... 🤙 @FredrikOseberg who may have more context
…tureEnvironmentVariants/PushVariantsButton/PushVariantsButton.tsx Co-authored-by: Nuno Góis <github@nunogois.com>
| padding: theme.spacing(0), | ||
| }, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
I think we can iterate on the details later, so we can have this ready to be shipped with 4.20. Also, maybe we should have a .styles.ts file as specified here?
| const [pushToAnchorEl, setPushToAnchorEl] = useState<null | HTMLElement>( | ||
| null | ||
| ); | ||
| const pushToOpen = Boolean(pushToAnchorEl); |
There was a problem hiding this comment.
It was some lines below, but I moved it up to keep the related components together... otherwise, you may think you need to use pushToAnchorEl for reading the state and never realize that there's a pushToOpen that should be used instead... which is a bit confusing IMO but 🤷
| } catch (e) { | ||
| throw e; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I believe the try catch throwing an exception without doing anything is pointless... I could just replace:
try {
const res = await makeRequest(req.caller, req.id);
return res;
} catch (e) {
throw e;
}with
return makeRequest(req.caller, req.id);and now there's no need to await because it's the return statement.
But it's specified that way in this ADR. Maybe the intent of the ADR was to say that you should do something with the exception? Or perhaps it's something we carried away from another place... 🤙 @FredrikOseberg who may have more context
This is what I plan to do, and I'll take in all comments in another PR. For others: do comment, I'll check them later |
About the changes
This adds the push to all button to the UI
Relates to roadmap item: #2254
UI