[GitHub] kristw commented on a change in pull request #5543: Reduce dashboard position_json data size

[GitBox <gi...@apache.org>]

kristw commented on a change in pull request #5543: Reduce dashboard position_json data size
URL: https://github.com/apache/incubator-superset/pull/5543#discussion_r207108701
 
 

 ##########
 File path: superset/assets/spec/javascripts/dashboard/util/findFirstParentContainer_spec.js
 ##########
 @@ -10,94 +10,91 @@ import {
 describe('findFirstParentContainer', () => {
   const mockGridLayout = {
     DASHBOARD_VERSION_KEY: 'v2',
-    DASHBOARD_ROOT_ID: {
-      type: 'DASHBOARD_ROOT_TYPE',
-      id: 'DASHBOARD_ROOT_ID',
-      children: ['DASHBOARD_GRID_ID'],
-    },
-    DASHBOARD_GRID_ID: {
-      type: 'DASHBOARD_GRID_TYPE',
-      id: 'DASHBOARD_GRID_ID',
-      children: ['DASHBOARD_ROW_TYPE-Bk45URrlQ'],
-    },
-    'DASHBOARD_ROW_TYPE-Bk45URrlQ': {
-      type: 'DASHBOARD_ROW_TYPE',
-      id: 'DASHBOARD_ROW_TYPE-Bk45URrlQ',
-      children: ['DASHBOARD_CHART_TYPE-ryxVc8RHlX'],
-    },
-    'DASHBOARD_CHART_TYPE-ryxVc8RHlX': {
-      type: 'DASHBOARD_CHART_TYPE',
-      id: 'DASHBOARD_CHART_TYPE-ryxVc8RHlX',
+    ROOT_ID: {
 
 Review comment:
   The first 3 lines seem to provide redundant information.
   
   ```js
   ROOT_ID: { // << key
     type: 'ROOT',  // same information that can be parsed from key
     id: 'ROOT_ID', // always same with key
     children: ['GRID_ID']
   }
   ```
   
   If we were to keep `type` as part of the ID, can we make `id = ${type}_${identifier}` a convention and get rid of `type` field in the object? 
   
   The `id` field value (e.g. ROOT_ID) is always equal to the `key`. Could we construct that if needed when parsing?
   
   ```js
   ROOT_ID: {
     children: ['GRID_ID']
   },
   GRID_ID: {
     children: ['CHART_123']
   }.
   CHART_123: {
     children: [] // this field seems redundant for leaf nodes. 
   } 
   ```
   or if these are stored as an array instead and converted to hash after parsing. 
   
   ```js
   [
     { id: 'ROOT_ID', children: ['GRID_ID'] },
     { id: 'GRID_ID', children: ['CHART_123']},
     'CHART_123' // plain string when no children or other metadata
   ]
   ```
   
   p.s. I haven't read the dashboard engine code so not sure how much change this will affect that code or are there other constraints? Just think that these approaches may save more space.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org