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

Update lab-grapher.js #12

Closed
wants to merge 0 commits into from

Conversation

rishiloyola
Copy link

I made changes in code to hide axis.
If you want to hide axis change opacity property to true.
If opacity property is false then axis are visible.

@knowuh
Copy link
Collaborator

knowuh commented Mar 13, 2015

A comment and a question:

  • "opacity" seems like the wrong name for that option, if setting it to true causes the axis to be transparent. I would call that flag something like "isTransparent" or "transparent", or even better "hidden".
  • Is there any reason to draw the components transparently add event listeners &etc., as opposed to simply not drawing them?

@sfentress
Copy link
Collaborator

Hi Rishi,

There are currently a few problems with this commit:

  1. As Noah says, the variable name doesn't really make sense (if something is opaque it means it is not transparent). Also, since you're adding this option to the top-level graph options, it's not clear it relates to the axes. Perhaps "hideAxisValues"?
  2. You've changed the default so that all graphs have hidden axis values by default. We would want the default to stay the same.
  3. By just making the values transparent, they still take up the same amount of space, even though they are invisible. A better option would be to simply never show the values. As @ddamelin suggested via email, a good place to start is by looking how the graph removes the axis values when in "responsive" mode (responsive: true). Specifically, I think you want to look at what happens whenever sizeType is less than 2. But note that we still want to show the axis labels.
  4. Finally, a quick reminder, remember that you need to change the code in lib/graph.js. This should get built into lab-grapher.js automatically, and then you can commit both.

Thank you!

@ddamelin
Copy link

Ultimately we'll also need changes in the Lab repo to make this affordance of the graph open to authors and part of our validation and sterilization processes. In particular the metadata.js associated with the interactive definition will need to be updated. Not sure what else.

@rishiloyola
Copy link
Author

Hi Sfentress,
Can you please explain your last point more,I am not getting your point.

@ddamelin
Copy link

Rishi, I've not built this code myself, but I think that the dist/lab-grapher.js is what is produced when you run the build script (see the readme.md). So to make changes in this codebase you make changes to the files in the lib directory and run the build script 'npm run build'

@sfentress
Copy link
Collaborator

@rishiloyola, Dan is correct, please look at the readme. If you have installed Node and run npm install in the project, you should be able to make changes in lib/graph.js, and then run npm run build and it will update the lab-grapher.js code.

Also, since you already have one pull request open on this issue, it would be best to keep new versions of your code in this PR. You can simply keep adding commits to the same branch, and this PR will update automatically. (When the code looks good, we can explain how to rebase the changes so that it all becomes one commit.) I will close the other PR.

@sfentress sfentress mentioned this pull request Mar 16, 2015
@sfentress
Copy link
Collaborator

Hi Rishi,

This looks generally better, thank you. There are two main issues:

  1. It's very hard to see exactly what has changed in the commit, because the line spacing has changed in lots of places. The indentation no longer follows the style of the rest of the library. This makes it hard to read the code (you have a lot of nested-if-statements that are unclear), and to know exactly what your changes are. Could you please recommit with the original formatting?
  2. The x-axis label now appears inside the graph area, as can be seen in this screenshot.

Thank you.

@ddamelin
Copy link

It would also be nice to have an example to demo this feature. Can you add a checkbox to the examples page, which toggles this feature on and off: https://github.com/concord-consortium/lab-grapher/blob/master/examples/index.html

@rishiloyola
Copy link
Author

Hi Sam,
The screen shot that you uploaded specifies which case? I mean responsible tick mark is on or off?
And I changed the formatting please have a look on it.

Thanks,
Rishi

@sfentress
Copy link
Collaborator

I still see the Year label inside the graph. It looks like you've moved it slightly. Do you not see the axis label in this location when you run it? Screenshot

@rishiloyola
Copy link
Author

Hi Sam,
Now test it. It is working fine on my system.
screenshot from 2015-03-17 22 20 21

@rishiloyola
Copy link
Author

Hi Dan,
I added a checkbox to the examples page, which toggles this feature on and off.Have a look on it.

@ddamelin
Copy link

Hi Rishi,

  • You fixed the spacing in the commit as Sam requested, but it still shows everything as deleted and then added again, with your changes mixed in. To fix this you need to start with a version of the file prior to your changes and then just add those changes without reformatting the entire file.
  • I still see overlap with the graph and x-axis label on my system. See screenshot here.
  • It would be great if you could set up Github pages so you can push you work to the gh-pages branch on your repo to generate an online version of the examples page that would make it easier for me to test. When set up properly I think the URL will be: http://rishiloyola.github.io/lab-grapher/examples/index.html#earth-surface-temperature. Info on Github pages is found here: https://help.github.com/categories/github-pages-basics/

@ddamelin
Copy link

Be sure to test with various browsers. It looks like the font spacing is different from browser to browser and that the method you are using to reposition the axis label is relying on this font size. If you look at the "small" size in the responsive layout option, then the axis values here don't seem to be affected by this problem, so you should take a look at how they are positioned.

@rishiloyola
Copy link
Author

Hello,
I made necessary changes in graph.js, please have a look at it.

@sfentress
Copy link
Collaborator

Hi Rishi, when I look at your gh-pages branch at http://rishiloyola.github.io/lab-grapher/examples/index.html#earth-surface-temperature, the Hide Axis Values button no longer hides the values, it just changes the font size.

Also, there are a lot of commits on this branch that go back and forth. Could you please rebase this branch to squash all your changes into one commit?

If you're using the command line, you should be able to do it like this:

  1. $ git rebase -i 537739e4832 // this is the id of the parent of your first commit
  2. Then, in the file that pops up, replace the word "pick" with the word "squash" in every line except the first one.
  3. Save and close the file.
  4. In the next file that pops up, edit your commit message.
  5. Check that you now have just one commit in that branch.
  6. If it all looks good, force push: git push origin hide-axis --force

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