Integrate chart to scalar with Vue #268

nickyfantasy · 2018-02-15T02:56:48Z

No description provided.

* use default prop instead of duplicate data and prop * use function instead of shorthand declaration with data

varunarora · 2018-02-15T22:11:10Z

frontend/src/scalars/ui/Chart.vue

+            </v-btn>
+            <v-select
+                v-if="downloadLink"
+                style="width:100px;"


If you have a fixed value, why not putting it in css

coz of laziness

varunarora · 2018-02-15T22:14:41Z

frontend/src/scalars/ui/Chart.vue

+    },
+    data() {
+        return {
+            width: 400,


Why are these stored as component data? Unless this changes (which we don't have right now right?), this should probably not be computed

Storing width and height in the data seems fine.

I think they put it here because user can expand/shrink the chart

varunarora · 2018-02-15T22:17:08Z

frontend/src/scalars/ui/Chart.vue

+            );
+            seriesOption = flatten(seriesOption);
+            let legendOptions = tagList.map(item => item.run);
+            let that = this;


let view = this; Who came up with that

lol, I coped all these from original san files, we actually do not need a local variable at all, just use this. for the rest of context

varunarora · 2018-02-15T22:19:37Z

frontend/src/scalars/ui/Chart.vue

+                };
+                return getPluginScalarsScalars(params);
+            });
+            axios.all(requestList).then(resArray => {


Requests should probably not be made from within component. Can you move this to a service / API manager outside?

varunarora · 2018-02-15T22:25:23Z

frontend/src/scalars/ui/Chart.vue

+        },
+
+        expandArea() {
+            let pageBoxWidth = document.getElementsByClassName('visual-dl-chart-page-box')[0].offsetWidth;


You should be using something like refs to find elements rendered by this component, instead of DOM query selector lookups.

https://vuejs.org/v2/guide/components.html#Child-Component-Refs

very good idea. ref is the way to go, I will replace all document.getElementsBy... with ref. However in this case it is finding the parent component element...let me see if there is a way to do it

jetfuel

Otherwise LGTM

jetfuel · 2018-02-20T18:34:02Z

frontend/src/scalars/Scalars.vue

@@ -39,14 +32,15 @@ import {getPluginScalarsTags, getRuns} from '../service';
 import {debounce, flatten, uniq, isArray} from 'lodash';
 import autoAdjustHeight from '../common/util/autoAdjustHeight';

-import Config from './ui/config'
+import Config from './ui/Config.vue'


You can also do import ui-config from './ui/Config.vue'
and simply do components: {
ui-config
}

hmm...compiler complains when I do import ui-config, "-" is not allowed in this case

jetfuel · 2018-02-20T18:34:55Z

frontend/src/scalars/Scalars.vue

@@ -119,12 +114,12 @@ export default {
            });
        },
        filteredConfig() {
-            let tansformArr = ['downloadLink', 'outlier'];
+            let transformArr = ['downloadLink', 'outlier'];


Good catch.

jetfuel · 2018-02-20T18:40:43Z

frontend/src/scalars/ui/Chart.vue

+const intervalTime = 15;
+
+export default {
+    components: {


If we don't plan to include any component, let's remove the components for now.

jetfuel · 2018-02-20T18:49:48Z

frontend/src/scalars/ui/Chart.vue

+    },
+    data() {
+        return {
+            width: 400,


Storing width and height in the data seems fine.

jetfuel · 2018-02-20T18:51:39Z

frontend/src/scalars/ui/Chart.vue

+        horizontal: function(val) {
+            this.setChartHorizon();
+        },
+        // runs: function(val) {


Why is this commented out?

They did not define setChartsRuns(line 102) in anywhere. But I expect they add this for future purpose, so I comment it for now.

jetfuel · 2018-02-20T18:59:26Z

frontend/src/scalars/ui/ChartPage.vue

+</template>
+<script>
+// import ExpandPanel from '../../common/component/ExpandPanel';
+import Chart from './Chart.vue';


I think it is possible to import without adding .vue

jetfuel · 2018-02-20T19:00:24Z

frontend/src/scalars/ui/Config.vue

@@ -1,26 +1,26 @@
 <template>


Is there a particular reason for the renaming?

Looks like Vue coding standard Component name starts with capital

jetfuel · 2018-02-20T19:02:52Z

frontend/src/scalars/ui/Config.vue

-        <v-slider label="Smoothing" v-bind:max="100" v-bind:min="0" v-model="config.smoothing" dark></v-slider>
-        <v-text-field v-model="config.smoothing" type="number" dark></v-text-field>
+        <div>
+            <v-slider label="Smoothing" :max="0.999" :min="0" :step="0.001" v-model="config.smoothing" dark></v-slider>


Does v-slider take step smaller than 1? I thought it only take Integer number

v-slider takes type Number which can be double or integer

* Initial checking to setup Vue basic requirements. (#264) Add App.vue Add AppMenu.vue * Vue with app menu (#265) * Add vuetify framework Update AppMenu.vue to include the basic menu item. * use name as key. * Add major components (#266) * Add vuetify framework Update AppMenu.vue to include the basic menu item. * use name as key. * Add Scalars, Images, Histograms, Graph. * Switch to use the stylus language for CSS * Port functions from Scalars.san to Scalars.vue * Add config vue (#267) * Add vuetify framework Update AppMenu.vue to include the basic menu item. * use name as key. * Add Config Vue * Integrate chart to scalar with Vue (#268) * Fix warnings from Vue compiler * use default prop instead of duplicate data and prop * use function instead of shorthand declaration with data * refract naming style, fix scalar options * Fix config UI * Add chart and chart page and integrate to scalar * update with varun and jeff's comments * Add images config (#270) * Add vuetify framework Update AppMenu.vue to include the basic menu item. * use name as key. * Add the Config.vue for Images * Add the CharPage.vue and Image.vue * Hook up the config to the image.vue so that the image height and width can update * Show Scalar Data and add ExpandPanel (#272) * Watch tagInfo changes to update chart and fix tooltip issue * Add expand panel and maintain isShow state in the panel * Add Expand Panel in Image * Add watch group name reg (#273) * Add vuetify framework Update AppMenu.vue to include the basic menu item. * use name as key. * Add Watch on the groupNameReg * Allow the nav bar to persist the selected item style. (#274) * Fix the incorrect pagination issue. (#276) * Add the Graph.vue and the Config.vue for Graph tab (#277) * Add the Histogram related vue files. (#278) * Fix error message on charts (#279) * Make sure to clear the chart before each use. * Add key for v-for loop. * Add theme and fix UI (#280) -Apply theme color in Vuetify -Add main.styl to override Vuetify default stylus variables -Add relative path in web pack config to find variables.styl * Fix multiple scalar issues: (#283) * Fix multiple scalar issues: -download link / outliers should take boolean value instead of array -download link now show proper selector of runsItem and download button -Use tag as key to prevent chart re-rendering -Add missing label in scalar config and fix UI * simply use index for key because scalar has different tag info structure from image and histogram * Fix font size and reorganize css and stylus (#287) * Fix comments * placeholder index.js * Fix issue where download data link points to empty run (#294) -Dropdown should only display non empty runs -Select first run by default -Remove drop down if all are empty runs -Refract download type to runItemForDownload -Add train run and multiple tags in mock data * Fix error when scalar data empty (#298) -Error comes in when trying to access elements inside empty data -When one of the runs has empty data, the chart will display nothing even other run has non empty data -Fix by checking length of data before accessing

nickyfantasy added 4 commits February 12, 2018 11:34

Fix warnings from Vue compiler

0773fd6

* use default prop instead of duplicate data and prop * use function instead of shorthand declaration with data

refract naming style, fix scalar options

c9d7c90

Fix config UI

b51b2ea

Add chart and chart page and integrate to scalar

ea238d8

nickyfantasy requested a review from jetfuel February 15, 2018 02:56

nickyfantasy assigned deqingli Feb 15, 2018

nickyfantasy requested a review from deqingli February 15, 2018 02:57

nickyfantasy unassigned deqingli Feb 15, 2018

nickyfantasy requested review from Superjomn and jacquesqiao February 15, 2018 03:21

varunarora reviewed Feb 15, 2018

View reviewed changes

jetfuel approved these changes Feb 20, 2018

View reviewed changes

update with varun and jeff's comments

32d4702

nickyfantasy merged commit 937a9e0 into PaddlePaddle:migrateFromSanToVue Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate chart to scalar with Vue #268

Integrate chart to scalar with Vue #268

nickyfantasy commented Feb 15, 2018

varunarora Feb 15, 2018

nickyfantasy Feb 20, 2018

varunarora Feb 15, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

varunarora Feb 15, 2018

nickyfantasy Feb 20, 2018 •

edited

varunarora Feb 15, 2018

varunarora Feb 15, 2018

nickyfantasy Feb 20, 2018

jetfuel left a comment

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

jetfuel Feb 20, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

jetfuel Feb 20, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

jetfuel Feb 20, 2018

nickyfantasy Feb 20, 2018

Integrate chart to scalar with Vue #268

Integrate chart to scalar with Vue #268

Conversation

nickyfantasy commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickyfantasy Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetfuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickyfantasy Feb 20, 2018 •

edited