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

Initial zoom level option in Visualizations #596

Merged
merged 4 commits into from
Nov 10, 2016

Conversation

RDCH106
Copy link
Contributor

@RDCH106 RDCH106 commented Nov 4, 2016

Default zoom level of the radata visualization (day , week, month, year) in the configuration panel.

By default the visualization uses week zoom. The zoom level selection is robust to unexpected values or undefined value.

It could be extended to other visualizations. I can prepare another PR to do that.

@Paul-Reed
Copy link
Member

A useful addition Ruben. It's really annoying to load up a dashboard of visualizations, to then have to manually select the preferred timeframes EVERY TIME!!!
Can I suggest that we close this PR, and add all the visualizations at the same time in a combined PR.

Paul

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 5, 2016

It is not needed. I can update the PR pushing the new changes.

@chaveiro
Copy link
Contributor

chaveiro commented Nov 5, 2016

A quick info: for multigraphs the default zoom level becomes the one that was active when the multigraph was saved.

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 7, 2016

  • Smoothie (Not needed)
  • BarGraph (Already has Interval configuration --> Pending)
  • Multigraph (Not needed)
  • Zoom (Not needed)
  • SimpleZoom (Not needed)
  • HistGraph (Not needed)
  • OrderTreshold (Not needed)
  • OrderBars (Not needed)
  • Stacked (Not needed)
  • StackedSolar (Not needed)

  • RealTime (Done)
  • RawData (Done)
  • TimeStoreDaily (Done)
  • Treshold (Done)
  • TimeCompare (Done)

*Only visualizations with selectable zooms are candidates to implement the "Initial Zoom"

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 7, 2016

In progress [###############] 15/15
Checking [COMPLETE]

@RDCH106 RDCH106 changed the title Default zoom level option in Rawdata Visualization Default zoom level option in Visualizations Nov 7, 2016
@chaveiro
Copy link
Contributor

chaveiro commented Nov 7, 2016

You can create a generic drop down for the user to select the zoom type from a predefined list with the optionsdata parameter in ..

@chaveiro
Copy link
Contributor

chaveiro commented Nov 7, 2016

.. vis_render.php. See the multigraph part for an example.

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 7, 2016

Thanks @chaveiro!
I will do.

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 7, 2016

@chaveiro Where is loaded vis_widget to use multigraphsDropBoxOptions?
I will need extra help...

@Paul-Reed
Copy link
Member

I tried to add a dropdown to this yesterday and hit the same issue....

@RDCH106 RDCH106 changed the title Default zoom level option in Visualizations Initial zoom level option in Visualizations Nov 8, 2016
@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 8, 2016

The Initial Zoom option is implemented in all the visualizations with selectable zooming.

Other visualizations can implement a similar concept but it is not the same concept. It could be "default zoom" or "time windows size" for example. It is the case of HistGraph, Treshold, OrderTreshold, OrderBars, Stacked, StackedSolar and TimeCompare. They could be implemented in other PR.

PR ready to merge!! ;-)

@Paul-Reed
Copy link
Member

Ruben, it would have been a nice refinement to add dropdowns to select the zoom type.
@chaveiro are you able to assist further please.

Paul

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 8, 2016

OK!
No problem with the proper help.

@chaveiro
Copy link
Contributor

chaveiro commented Nov 9, 2016

Hi, sorry late reply.

So you can add the dropdown content by adding this to the vis_render.php file at each widget config:

"optionsdata": [[1:"Day"],[7:"Week"],[30:"Month"],[365:"Year"]],

Then act on the numeric value that is passed to the widget accordingly.

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 10, 2016

Nothing!
Error adding the line in each file...
Unexpected ":" symbol

If we merge the PR and we fix it later?

I am not going to spend more time on it. Sorry!

Now it works and it is better than before. I cannot achieve the refinement to add dropdowns.

@chaveiro
Copy link
Contributor

Question, with this PR will it be able to define initial zoom for a graphical widget on a dashboard?

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 10, 2016

Yes @chaveiro , I tested it with all the implementations.
All the summary and checkings are in the previous comments.

And they also pass the Travis and Codacy checkings.

@Paul-Reed
Copy link
Member

Tested here too. Works as expected, and initial setting persists within the
dashboard.

On 10 Nov 2016 13:13, "Rubén de Celis Hernández" notifications@github.com
wrote:

Yes @chaveiro https://github.com/chaveiro , I tested it with all the
implementations.
All the summary and checkings are in the previous comments.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#596 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA7bDNTtg8MoR8P_KOnqK7YAIeRub-nSks5q8xhogaJpZM4KpcJu
.

@chaveiro
Copy link
Contributor

chaveiro commented Nov 10, 2016

Ok, nice.
Regarding the problem you have to change the optionstype to dropbox also and fix any miss placed characters: brackets, dashes.. that is javascript syntax. Try something like this

"optionstype":["dropbox"],
"optionsdata": [["1":"Day"],["7":"Week"],["30":"Month"],["365":"Year"]],

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 10, 2016

@chaveiro I already tried it, without success...

@Paul-Reed
Copy link
Member

Would it help of the changes made so far were merged, then maybe look at
this via further PRs?

On 10 Nov 2016 14:03, "Rubén de Celis Hernández" notifications@github.com
wrote:

@chaveiro https://github.com/chaveiro I already tried it, without
success...


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#596 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA7bDPDTAwfWBBwq6Wn19eN-U8ykpnr8ks5q8yQ-gaJpZM4KpcJu
.

@chaveiro chaveiro merged commit b1182b3 into emoncms:master Nov 10, 2016
@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 10, 2016

Thanks for the merge!!!

@chaveiro
Copy link
Contributor

chaveiro commented Nov 10, 2016

I've made my refactoring on top of your changes, please take a look and start from that.
Changes
Added: Realtime
Refactored: timestoredaily, threshold, Rawdata
Reverted: bargraph (already had Interval to specify default zoom)

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 10, 2016

I am not agree with "Reverted: bargraph (already had Interval to specify default zoom)"
Check my revision in the merge #597

@Paul-Reed
Copy link
Member

605 mins Zoom for Rawdata?
date

@Paul-Reed
Copy link
Member

...also agree with @RDCH106 about bargraph.
The 'interval' is not the same thing as the initial scale.

Paul

@chaveiro
Copy link
Contributor

chaveiro commented Nov 10, 2016

All right, no problem about bargraph, want to implement again (now with the new formatting) ?
Ops that '605 min' should read "Day". Fixed at 606c6a7

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 11, 2016

Yes, I will do it again with the new formatting.
Thanks @chaveiro and thanks @Paul-Reed for your checkings!

@chaveiro
Copy link
Contributor

-> Added default zoom to timecompare #600
I think almost all widgets would benefit from having this.
Are you able to implemment on the resting ones?

@RDCH106
Copy link
Contributor Author

RDCH106 commented Nov 14, 2016

@chaveiro Restored Zoom option in BarGraph visualization #604

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