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

Replace all visualizations with dds.js #291

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Replace all visualizations with dds.js #291

wants to merge 23 commits into from

Conversation

Gerrrr
Copy link
Collaborator

@Gerrrr Gerrrr commented Feb 10, 2016

Solution for: #289

@codecov-io
Copy link

Current coverage is 94.76%

Merging #291 into master will not affect coverage as of 550b1fe

@@            master    #291   diff @@
======================================
  Files           20      20       
  Stmts         1185    1185       
  Branches       147     147       
  Methods          0       0       
======================================
  Hit           1123    1123       
  Partial          0       0       
  Missed          62      62       

Review entire Coverage Diff as of 550b1fe

Powered by Codecov. Updated on successful CI builds.

@FRosner
Copy link
Owner

FRosner commented Feb 11, 2016

@Gerrrr I think codacy will complain a lot less if you name your dds.js dds.min.js.

@Gerrrr
Copy link
Collaborator Author

Gerrrr commented Feb 11, 2016

@FRosner done

@FRosner
Copy link
Owner

FRosner commented Feb 11, 2016

@Gerrrr except for the one remaining issue in Visualization.js codacy is happy. I will check the branch out, build it and give it a try in the next days. Thanks a lot!

@@ -44,8 +44,6 @@
*/
define(function(require) {
Copy link
Owner

Choose a reason for hiding this comment

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

Shiiiieet codacy... Are you never happy? :D

Shall we lock her up somewhere, @Gerrrr ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it actually has a point - we don't use require in this function. I'll fix it later today.

@Gerrrr
Copy link
Collaborator Author

Gerrrr commented Feb 16, 2016

@FRosner, PR is ready for review, please ignore codacy complaints.

@FRosner
Copy link
Owner

FRosner commented Feb 17, 2016

Thanks for the pull request @Gerrrr. After executing the core/src/test/resources/manual-tests.txt I have the following comments. Please find the respective line numbers in the headlines of each issue.

Canvas Size (L: 14, 15, ...)

  • The canvas is very small. When you move the nodes in a graph a bit, you quickly reach the end. Then the nodes are lost and you cannot select them. With tables we force the user to scroll a lot. All visualizations should take as much space as there is available in their parent div, unless there is a good reason not to.

image

image

Dashboard Blocking the UI (L: 19)

  • When I select a dashboard visualization, the browser becomes unresponsive. After some time, I don't see the full dashboard but only a table.

Summary Composite Overplotting (L: 20)

  • The composite visualizations overplot.

image

  • When I view on mobile screen proportions, I even get this:

image

Heatmap with NaN not Displayed (L: 21)

  • The heatmap just shows an empty screen, probably because of the NaN value (which should be shown black).

Scatterplot (L: 26)

  • Didn't you say that you went for the C3 scatter plot? Is it already included or still my crappy homegrown one?
  • Also the jitter button seems to be wrong. It shows jitter when there is none and the other way round. Probably just an inverted switch.

Key Value Pairs (L: 27)

  • It looks pretty stuffed in the corner.

image

Table Not Drawn (L: 97)

  • When I draw the huge table containing all possible column types, it does not draw anything.

Visualization Title Positioning

  • The title is also pretty stuffed in the corner. Some whitespace would help :)

Graph Buttons Wrong (L: 14)

  • The buttons for triggering directed edge drawing and edge label drawing are inverted.

image

Graph Redrawing

  • When clicking buttons on the graph, it gets redrawn starting from random positions for each node. This creates a lot of visual noise. Instead, it should just change the property required (e.g. hide edge labels).

Overall Performance

  • All in all, the UI seems slower than before. Are there ways to tune it?

@@ -0,0 +1,21 @@
define(function(require) {
Copy link
Owner

Choose a reason for hiding this comment

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

@Gerrrr
Copy link
Collaborator Author

Gerrrr commented Mar 8, 2016

I fixed the issues you mentioned above. Can you please review it again?

@FRosner
Copy link
Owner

FRosner commented Mar 24, 2016

I released DDQ 3.0.0 today and will take a look at it in the next days @Gerrrr. Thanks!

@FRosner
Copy link
Owner

FRosner commented Mar 25, 2016

WIP

  • [info] Loading /Users/frosner/Documents/projects/spawncamping-dds/web-ui/src/main/resources/ui/app/require.config.js Could not read file: /Users/frosner/Documents/projects/spawncamping-dds/web-ui/src/main/resources/ui/app/require.config.js error was: InternalError: Couldn't read source file "./../lib/d3.v3.min.js": ./../lib/d3.v3.min.js (No such file or directory). (rjs/r-2.0.1.js#2110)
  • Edge labels are off
    image
  • Two scroll bars for table (one for the table and one useless one for the complete page). The scroll bar for show(sqlGolf) just scrolls into nirvana.

image

  • dashboard(sqlFlights.select("flightDate", "carrier", "carrierDelay")) breaks my browser...

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.

3 participants