Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yas created an issue. See original summary.

yas’s picture

Status: Needs work » Needs review
FileSize
18.63 KB
yas’s picture

@xiaohua-guan
@baldwinloue
@masami

Could you please review the patch?

Here is the screenshot:

screenshot-20191003a.png

To use this patch, clear cache and add a block named K8s Node Heatmap.

Xiaohua Guan’s picture

@yas

I have some questions about the patch.

1. The "??" seems typo.

+      $memory = $message[$cloud_context]['nodes'][$node_name]['memory'] ?? 0;

2. The parameter $request isn't being used, so it would be better to remove it.

+  public function getNodeHeatmapData(Request $request, $cloud_context) {

3. There is another way to add "k8s/d3" as the dependency library of the k8s/k8s_node_heatmap in k8s.libraries.yml.

+          'k8s/d3',
+          'k8s/k8s_node_heatmap',

4. I can't find the code to include the block to node list view. Maybe I missed something.

baldwinlouie’s picture

@yas, I have the following comments.

  1. +++ b/modules/cloud_service_providers/k8s/src/Plugin/Block/K8sNodeHeatmapBlock.php
    @@ -0,0 +1,145 @@
    +    $cloud_context = \Drupal::service('current_route_match')->getParameter('cloud_context');
    

    current_route_match should be changed to dependency injection.

  2. +++ b/modules/cloud_service_providers/k8s/src/Plugin/Block/K8sNodeHeatmapBlock.php
    @@ -0,0 +1,145 @@
    +    $route_name = \Drupal::routeMatch()->getRouteName();
    

    routeMatch() should be changed to dependency injection

  3. +++ b/modules/cloud_service_providers/k8s/src/Plugin/Block/K8sNodeHeatmapBlock.php
    @@ -0,0 +1,145 @@
    +    \Drupal::service('page_cache_kill_switch')->trigger();
    

    Can be switched to dependency injection

  4. +++ b/modules/cloud_service_providers/k8s/src/Plugin/Block/K8sNodeHeatmapBlock.php
    @@ -0,0 +1,145 @@
    +    return \Drupal::moduleHandler()->moduleExists('big_pipe') ? 1 : 0;
    

    Can be switched to dependency injection

  5. +++ b/modules/cloud_service_providers/k8s/src/Plugin/cloud/server_template/K8sCloudServerTemplatePlugin.php
    @@ -208,7 +221,12 @@ class K8sCloudServerTemplatePlugin extends PluginBase implements CloudServerTemp
    +      'data' => $this->entityLinkRenderer->renderViewElement(
    +        $entity->get('field_namespace')->value,
    +        'k8s_namespace',
    +        'name',
    +        []
    +      ),
    

    This code seems to be left over from https://www.drupal.org/project/cloud/issues/3085381

    This shouldn't be part of the patch.

  6. +++ b/modules/cloud_service_providers/k8s/js/k8s_node_heatmap.js
    @@ -0,0 +1,167 @@
    +  // Create a dummy <span>...</span> to measure the string pixel length.
    +  $('<span/>', {id: 'width_check', appendTo: 'body'})
    +    .css('font-size', FONT_SIZE_AXIS_LABEL)
    +    .css('visibility', 'hidden')
    +    .css('position', 'absolute')
    +    .css('white-space', 'nowrap');
    

    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?

yas’s picture

@xiaohua-guan
@baldwinlouie

Thank you for your review. I fixed the patch.

For comment #4,

  1. It is a Null Coalescing Operator.
  2. I remove the parameter Request $request.
  3. I included k8s/d3 into dependencies in k8s.libraries.yml.
  4. I didn't include that block the node list view by default.

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:

screenshot-20191003a_0.png

Xiaohua Guan’s picture

@yas

It looks good to me now. Thanks.

yas’s picture

@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?

baldwinlouie’s picture

Status: Needs review » Reviewed & tested by the community

@yas, looks good to me

yas’s picture

@baldwinlouie

Thank you for your review. I'll merge the patch to 8.x-2.x and close this issue as Fixed.

  • yas committed cc465cb on 8.x-2.x
    Issue #3085699 by yas, Xiaohua Guan, baldwinlouie: Add a heatmap to...
yas’s picture

Status: Reviewed & tested by the community » Fixed
yas’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.