refactor(site): refactor workspace notifications #11520
Conversation
| } | ||
|
|
||
| const infoNotifications = notifications.filter((n) => n.severity === "info"); | ||
| const warningNotifications = notifications.filter( |
There was a problem hiding this comment.
I'm wondering if it's practical/a good idea to try splitting up the notifications logic into a helper function. Something like this:
function getNotifications (...inputs) {
/** Main logic goes here */
return {
info: [...],
warnings: [...],
};
}My main worry is that a lot of the branches are kind of interspersed throughout the component, with a random useEffect call wedged between them
There was a problem hiding this comment.
I'm not sure if I got it 🤔
There was a problem hiding this comment.
I know the code has changed a little bit since the comment, but I'm going to go with the code on this PR as an example. The way I see the WorkspaceNotifications component, it kind of has different sections:
export const WorkspaceNotifications = (props) => {
<-- Props destructuring -->
<-- Set up empty notifications array -->
<-- Calculate values from workspace query -->
<-- Mutate notifications array if outdated -->
<-- Mutate notifications array if unhealthy -->
<-- Set up state for checking whether actions are enabled -->
<-- Mutate notifications array if actions are enabled -->
<-- Set up state/effects for alerts -->
<-- Mutate notifications array if alerts are pending -->
<-- Mutate notifications array if template is deprecated -->
<-- Mutate notifications array if template is deprecated -->
<-- Split the notifications array by warning/info type -->
<-- Return JSX output -->
};Most of the sections are dealing with calculating the notifications array
export const WorkspaceNotifications = (props) => {
<-- Props destructuring -->
+ <-- Set up empty notifications array -->
<-- Calculate values from workspace query -->
+ <-- Mutate notifications array if outdated -->
+ <-- Mutate notifications array if unhealthy -->
<-- Set up state for checking whether actions are enabled -->
+ <-- Mutate notifications array if actions are enabled -->
<-- Set up state/effects for alerts -->
+ <-- Mutate notifications array if alerts are pending -->
+ <-- Mutate notifications array if template is deprecated -->
+ <-- Mutate notifications array if template is deprecated -->
+ <-- Split the notifications array by warning/info type -->
<-- Return JSX output -->
};But the notifications array should always be the same if we have the same inputs, so seemingly, there should be a way to extract it into a reusable function. If we had the function, we could reorganize things like this:
export const WorkspaceNotifications = (props) => {
<-- Props destructuring -->
<-- Calculate values from workspace query -->
<-- Set up state for checking whether actions are enabled -->
<-- Set up state/effects for alerts -->
+ <-- Call function to get notifications -->
<-- Return JSX output -->
};The function might still mutate an array under the hood to fill it with data, but as long as we don't have any side effects, it will be fine, and it might be easier to add more cases for adding extra notifications down the line
There was a problem hiding this comment.
But for some of them, we need to fetch data... so how to deal with that without any effect?
There was a problem hiding this comment.
We would still have the effects and fetching – that's why I didn't remove the non-notification steps. It's just that their values would get passed into the function on each render
I did just realize I forgot the query section, though. It should be:
export const WorkspaceNotifications = (props) => {
<-- Props destructuring -->
+ <-- Set up workspaces autostart query state -->
<-- Calculate values from workspace query -->
<-- Set up state for checking whether actions are enabled -->
<-- Set up state/effects for alerts -->
+ <-- Call function to get notifications -->
<-- Return JSX output -->
};|
I worry that hiding them in the corner will make these a bit too low visibility. Whenever an app hides the "new" button or similar in that corner I never see it, and always get frustrated when I do eventually figure it out. I'm thinking maybe these should go in the new "top bar" area, next to the action buttons and build status. A bit more noticeable, but I love the little badge design, and how much more space efficient it is! |
|
@aslilac in the topbar, I think those notification pills can look "too much" since we already have the status pill there 🤔 but I will play around with this idea |
Motivation
Sometimes, displaying workspace notifications as alerts can be annoying and distract users from accessing the workspace/agent apps and data.
Solution
To solve that, I proposed a design a few months ago to improve that by grouping them behind notification pills on the bottom right of the page.
Implementation details
Screen.Recording.2024-01-09.at.14.12.49.mov
Related to #11491