Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- Add a block to display a heatmap for the Pods allocation on each node
Comment | File | Size | Author |
---|---|---|---|
#8 | 3085699-8.patch | 20.27 KB | yas |
Comments
Comment #2
yasComment #3
yas@xiaohua-guan
@baldwinloue
@masami
Could you please review the patch?
Here is the screenshot:
To use this patch, clear cache and add a block named
K8s Node Heatmap
.Comment #4
Xiaohua Guan CreditAttribution: Xiaohua Guan commented@yas
I have some questions about the patch.
1. The "??" seems typo.
2. The parameter $request isn't being used, so it would be better to remove it.
3. There is another way to add "k8s/d3" as the dependency library of the k8s/k8s_node_heatmap in k8s.libraries.yml.
4. I can't find the code to include the block to node list view. Maybe I missed something.
Comment #5
baldwinlouie CreditAttribution: baldwinlouie commented@yas, I have the following comments.
current_route_match should be changed to dependency injection.
routeMatch() should be changed to dependency injection
Can be switched to dependency injection
Can be switched to dependency injection
This code seems to be left over from https://www.drupal.org/project/cloud/issues/3085381
This shouldn't be part of the patch.
Will this algorithm to measure width work if there is blocks in the main content region as well as something in the right hand column region?
Comment #6
yas@xiaohua-guan
@baldwinlouie
Thank you for your review. I fixed the patch.
For comment #4,
Request $request
.k8s/d3
into dependencies ink8s.libraries.yml
.For comment #5,
1-4. I changed the function calls to dependency injection.
5. Thank you for your finding, I'm sorry for my mistake.
6. I think _basically_ it should behave as we expect:
Comment #7
Xiaohua Guan CreditAttribution: Xiaohua Guan commented@yas
It looks good to me now. Thanks.
Comment #8
yas@xiaohua-guan
Thank you for your review.
i fixed the patch since it still contains the prior merge.
@baldwinlouie
Could you please review the patch again?
Comment #9
baldwinlouie CreditAttribution: baldwinlouie commented@yas, looks good to me
Comment #10
yas@baldwinlouie
Thank you for your review. I'll merge the patch to
8.x-2.x
and close this issue asFixed
.Comment #12
yasComment #13
yas