feat(site): move resources into the sidebar #11456
Conversation
|
Related to #11491 |
site/src/utils/workspace.tsx
Outdated
| // These resources (i.e. docker_image, kubernetes_deployment) map to Terraform | ||
| // resource types. These are the most used ones and are based on user usage. | ||
| // We may want to update from time-to-time. | ||
| const BUILT_IN_ICON_PATHS: Record<string, string> = { |
There was a problem hiding this comment.
Is it safe to assume that all the icons should start with /icon/? I'm wondering if there might be value in redefining the record type as
Record<string, `/icon/${string}`>Just to help catch accidental typos
There was a problem hiding this comment.
love the idea
| </p> | ||
| )} | ||
| {resources.length === 0 && | ||
| !failed && |
There was a problem hiding this comment.
Is this condition strictly for handling loading states?
| display: "flex", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| lineHeight: 0, |
There was a problem hiding this comment.
Do you want a lineHeight of 0, or a lineHeight of 1? I've had issues where a value of 0 broke a lot of text rendering and made text invisible the moment something accidentally overflowed to a new line
There was a problem hiding this comment.
I think 1 🤔 but let me check.
There was a problem hiding this comment.
Ahh in this case, it is an image wrapper so it is ok to use 0... I'm going to replace those wrappers - we have a few in the codebase - with the new @aslilac component ExternalIcon
|
|
||
| export const ResourceSidebarItemSkeleton = () => { | ||
| return ( | ||
| <div css={[styles.root, { pointerEvents: "none" }]}> |
There was a problem hiding this comment.
Does pointerEvents do anything here? Divs aren't interactive by default, and turning on the style rule would just make clicks on the div hit the parent directly, which wouldn't be that different from the events firing on the div, and then bubbling up to the parent
There was a problem hiding this comment.
It is because it has the root style which applies a "hover" style. I just found it easier to set pointerEvents to none to remove that. Do you see better ways?
| padding: 2, | ||
| }} | ||
| > | ||
| <img |
There was a problem hiding this comment.
Can you add role="presentation" here?
There was a problem hiding this comment.
Empty alt has similar behavior to the presentation role.
In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers. Text values for these types of images would add audible clutter to screen reader output or could distract users if the topic is different from that in adjacent text. Leaving out the alt attribute is also not an option because when it is not provided, some screen readers will announce the file name of the image instead.
| }; | ||
|
|
||
| const selectedResourceId = useTab("resources", ""); | ||
| const resources = workspace.latest_build.resources.sort( |
There was a problem hiding this comment.
Not sure if I got it 🤔
There was a problem hiding this comment.
So, the .sort method is mutating the underlying array, even though that array is globally available throughout the app through the query cache. That could cause bugs for any other components that need the data in its original form
I don't think the .toSorted method is supported enough to swap that in, but you should be able to rewrite this as
const resources = [...workspace.latest_build.resources].sort()
Screen.Recording.2024-01-05.at.14.34.35.mov