[11513] Added workload-dashboard#17728
Conversation
marcelofukumoto
changed the title
| @@ -0,0 +1,561 @@ | |||
| <script> | |||
There was a problem hiding this comment.
Can we ensure that new components are written in typescript, where possible?
There was a problem hiding this comment.
Good point!
Let me change to CompositionAPI as well.
| fetchAll: Failed to fetch workload summaries | ||
| empty: | ||
| title: No workloads to show | ||
| message: "Tips: undo the last namespace filter you applied or <resetLink>reset the namespaces filter</resetLink>." |
There was a problem hiding this comment.
this use case is different, the user may have not set any and the default only user namespaces is used
| message: "Tips: undo the last namespace filter you applied or <resetLink>reset the namespaces filter</resetLink>." | |
| message: "Tips: Update the namespace filter above or <resetLink>reset the namespaces filter</resetLink>." |
There was a problem hiding this comment.
This one is provided by @oboc-sts from the Figma.
Let me ask him to check here directly.
There was a problem hiding this comment.
I totally missed the "suggested change"...
Yeah, we can go with the suggestion @richard-cox made above, it makes sense, I like it ;)
| import { StateColor } from '@shell/utils/style'; | ||
| import { computed } from 'vue'; | ||
| import { useStore } from 'vuex'; | ||
| import type { RouteLocationRaw } from 'vue-router'; |
There was a problem hiding this comment.
@codyrancher would you be able to review these component updates / new components? your composition api fu is greater than mine
| </script> | ||
|
|
||
| <template> | ||
| <div |
There was a problem hiding this comment.
we need to restrict the max width on these to avoid scenarios where the label is miles away from the value
could have the box the same size bit limit the label-->value gap? or just restrict the box? will leave that with you and UX
There was a problem hiding this comment.
same also applies to the by type cards (though not as bad)
There was a problem hiding this comment.
original comment resolved. we can monitor the second and address later on
There was a problem hiding this comment.
What's the resolution on these 2 points (max-width for the page content = byState and byType cards) ?
I had a conversation with @marcelofukumoto some time ago and I remember we touched upon this topic...
There was a problem hiding this comment.
For 3 columns it turned out like this:
For 2 columns like this:
There was a problem hiding this comment.
I think you've pasted the wrong screenshot :P (I hope at least :D )
There was a problem hiding this comment.
Right! Sorry! Updated correctly now!
| <!-- ━━━ By State ━━━ --> | ||
| <div class="section"> | ||
| <h4 class="m-0 text-muted"> | ||
| {{ t('workloadDashboard.sections.byState') }} |
There was a problem hiding this comment.
should these be muted? they look a little hidden in dark mode
There was a problem hiding this comment.
You are right! That byState is technically the deemphasized one. Changed.
There was a problem hiding this comment.
@marcelofukumoto can the 2 of us review everything once again after the code review has been finished? I want to make sure we haven't deviated too much from the last conversation we had. Feel free to send an invite at your convenience...
There was a problem hiding this comment.
Sure! As soon as we finish here we should have a final review before the merge.
| } | ||
| } | ||
|
|
||
| :deep(.state-card) { |
There was a problem hiding this comment.
is there anyway we can avoid this :deep?
There was a problem hiding this comment.
Hmm Ok! Added as a prop.
There was a problem hiding this comment.
Removed the usage of GAP as well and changed to margin. Than I dont need the deep.
|
|
||
| .state-card { | ||
| border: 0; | ||
| :deep(.resource-row) { |
There was a problem hiding this comment.
:deep
think this one would benefit from a propin the StateCard component. something like sparse?
can also position the state dot (left / right) with a prop (stateDotPostition)
There was a problem hiding this comment.
I called it compact and added to the ResourceRow, following some of the line-height and other things from Figma.
| &--full { | ||
| grid-column: 1 / -1; | ||
|
|
||
| :deep(.body) { |
There was a problem hiding this comment.
Ok! Moved to a prop!
|
|
||
| await Promise.all(batch.map(async([stateKey, { originalName, type }]) => { | ||
| try { | ||
| const url = `/v1/${ type }?limit=1&filter=metadata.state.name=${ originalName }`; |
There was a problem hiding this comment.
| const url = `/v1/${ type }?limit=1&filter=metadata.state.name=${ originalName }`; | |
| const url = `/v1/${ type }?pagesize=1&filter=metadata.state.name=${ originalName }`; |
limit is a native kube api param and doesn't work with vai requests. the vai filter means we're going through a different path and so we need to use pagesize instead
though i would go down the path of using steve-pagination-utils createParamsForPagination to create the url here as well. that keeps this code nicely isolated from that shenanigans
| const all = store.getters['prefs/get'](STATE_COLOR_MAP) || {}; | ||
| const current = all[clusterId.value] || {}; | ||
|
|
||
| store.dispatch('prefs/set', { |
There was a problem hiding this comment.
this just caches in vuex (preference isn't stored in a cookie or via api), so we don't really get anything above caching globally in this file?
There was a problem hiding this comment.
Agreed. So that means I can just cache in the file? Seems ok for me!
Changed to only cache in the file.
| const stateColorMap = computed<Record<string, StateColor>>(() => { | ||
| const all = store.getters['prefs/get'](STATE_COLOR_MAP) || {}; | ||
|
|
||
| return all[clusterId.value] || {}; |
There was a problem hiding this comment.
we've got stuck caching things by cluster before (without manually clearing out entries that no longer exist). lets gamble and make them global
There was a problem hiding this comment.
Since removed the prefs/set usage at all. All good. Not going to have that anymore
marcelofukumoto
force-pushed
the
11513-replace-workloads-page-list-with-an-overview-page
branch
2 times, most recently
from
dff7069 to
201d7bd
Compare
| showPercent?: boolean; | ||
| noResourcesMessage?: string; | ||
| to?: RouteLocationRaw; | ||
| rowTo?: RouteLocationRaw | string; |
There was a problem hiding this comment.
RouteLocationRaw already includes string, so | string appears to be redundant.
There was a problem hiding this comment.
Not using this anymore! Thanks
| props.resources?.forEach((resource: any) => { | ||
| accumulator[resource.stateDisplay] = accumulator[resource.stateDisplay] || { count: 0 }; | ||
| accumulator[resource.stateDisplay].count++; | ||
| accumulator[resource.stateDisplay] = accumulator[resource.stateDisplay] || { count: 0, stateId: resource.stateId || resource.stateDisplay }; |
There was a problem hiding this comment.
I believe that falling back on stateDisplay for stateId could cause a problem since the stateId is used to create query strings. I don't think In Progress in the query string is what was intended.
There was a problem hiding this comment.
Not using stateDisplay anymore! Only the ID
| return; | ||
| } | ||
|
|
||
| router.push(to); |
There was a problem hiding this comment.
The user can't select text with this change. Upon release the mouse the user will navigate to the next page.
There was a problem hiding this comment.
Fixed this one! Thanks!
There was a problem hiding this comment.
I have a feeling you may want to create a new component rather than extending this card and the status card.
This was largely intended to implement the summary cards in the design system but we haven't moved it into rancher components yet. https://www.figma.com/design/K3e7DHHM5TsS0vQELeoIFZ/Rancher-DS?node-id=676-159&m=dev
I think we may want to create a new card with a new intent. I think you could move and reuse some of the useful bits from status card and create the new one.
There was a problem hiding this comment.
I will create a new component! I think that is the best for now. And I will fix the other issues on the new component
There was a problem hiding this comment.
Created new components. A lot cleaner. The components have no complexity and as you shared the previous components were not created to be shared. Reduced the files changed as well.
| class="detail-card" | ||
| :class="{ clickable: clickable }" | ||
| :role="clickable ? 'link' : undefined" | ||
| :tabindex="clickable ? 0 : undefined" | ||
| @click="handleClick" | ||
| @keyup.enter="handleClick" |
There was a problem hiding this comment.
It looks like we're missing relevant accessibility attributes. Maybe aria-label and role.
There was a problem hiding this comment.
Improved accessibility! Thanks for the tip.
|
@richard-cox there are big changes to the components since I moved to not use the Card and StatusCard Then avoiding all the props or :deep to make it work. The code is a lot cleaner for sure. The only component I decided to keep is the SubtleLink since it is used on some special places and I added the icon with the open in new tab only. The code is ready to be reviewed again. |
5 participants
Summary
Fixes #11513
Occurred changes and/or fixed issues
Technical notes summary
Areas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
Checklist
Admin,Standard UserandUser Base