Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from san to vue #281

Merged
merged 26 commits into from
Mar 8, 2018
Merged

Migrate from san to vue #281

merged 26 commits into from
Mar 8, 2018

Conversation

jetfuel
Copy link
Collaborator

@jetfuel jetfuel commented Feb 24, 2018

We created Vue version of the VisualDL. Please take some time to take a look. Once it is merged, let's discuss how we can build interactive graph together.

jetfuel and others added 14 commits February 8, 2018 15:09
* Add vuetify framework
Update AppMenu.vue to include the basic menu item.

* use name as key.
* 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 vuetify framework
Update AppMenu.vue to include the basic menu item.

* use name as key.

* Add Config Vue
* 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 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
* 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 vuetify framework
Update AppMenu.vue to include the basic menu item.

* use name as key.

* Add Watch on the groupNameReg
* Make sure to clear the chart before each use.

* Add key for v-for loop.
-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
nickyfantasy and others added 4 commits February 27, 2018 13:58
* 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
},
methods: {
handleHeadClick() {
this.toogleShow();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toggle

<div class="visual-dl-expand-panel">
<h3
class="visual-dl-expand-head"
@click="handleHeadClick()"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this click only toggles show/hide, we can write it simply as below:

@click="isShow = !isShow"

@@ -1,7 +1,9 @@
import Notification from './Notification';
import NotificationItem from './NotificationItem';
//TODO: Migrate this part to Vue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this file be moved?


getOriginChartsData() {
getPluginGraphsGraph().then(({status, data}) => {
if (+status === 0 && data.url) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the '+' an error?
Also we should add handler for error promise later


export default {
props:['config'],
data() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data() is not used thus should be removed.

@daming-lu
Copy link
Collaborator

screen shot 2018-03-02 at 5 16 37 pm

The download button toggled by 'Show data download link' is broken. Note the small download button (4th one on the upper row) is working

@nickyfantasy
Copy link
Contributor

@daming-lu good catch regarding the download button bug. This is because "train" run is empty and nothing can be downloaded, same issue before the migration. I think the fix should let the selector only displays non empty chart.

-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
daming-lu and others added 3 commits March 6, 2018 15:50
-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
},
methods: {
handleItemClick: function (item) {
this.selected = item.name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里为何没有缩进,建议合入后改一下,小问题

Download image
</v-btn>

<v-slider label="Scale"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 label 属性可以写在下一行,格式统一

<script>
import ExpandPanel from '../../common/component/ExpandPanel';
import Image from './Image';
//import Pagination from 'san-mui/Pagination';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

san-mui 相关的后面统一删了吧

Copy link
Contributor

@deqingli deqingli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jetfuel jetfuel merged commit a6b80b9 into develop Mar 8, 2018
@jetfuel jetfuel deleted the migrateFromSanToVue branch March 8, 2018 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants