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

Fixed bug #34

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

Fixed bug #34

wants to merge 2 commits into from

Conversation

DylanBulmer
Copy link

I fixed the bug I encountered in isssue #33

@DylanBulmer
Copy link
Author

I only changed the file within the src folder, I didn't want to change the items in the dist folder.

@kamlekar
Copy link
Owner

kamlekar commented May 4, 2018

Hi @DylanBulmer ,

Thanks for the PR. much appreciated!!

Can you please change your PR to not remove variable declaration from everywhere? I don't want to have conflicts with other global variables from somewhere else because of addition of this library.

@DylanBulmer
Copy link
Author

I'm not changing any global variables I thought. I added in a comment in the beginning to help the intelli-sense let me know how to handle different parts of the code. I removed some unnecessary parentheses and the var keyword that was in front of already declared variables from the parameter lists.

I can re-upload the code without those changes if you'd like.

Copy link
Author

@DylanBulmer DylanBulmer left a comment

Choose a reason for hiding this comment

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

I only tabbed an un-indented piece of code

@kamlekar
Copy link
Owner

kamlekar commented May 5, 2018

@DylanBulmer you aren't changing any global variables but if the user's project has global variable e, that will lead to conflicts. It is better to write the library code specifically always. Can you please leave the var as it is?

Edit: I understood now. Sorry for the confusion. you did right. Please check my latest comment about test failing.

@kamlekar
Copy link
Owner

kamlekar commented May 5, 2018

Hi @DylanBulmer just tested the code, the test1 is failing. FYI, there are two test folders which needed to pass for me to accept this PR.

Please check by:

  • Moving the scrollbar (with mousewheel)
  • Clicking in the scroll region, to move the scroll bar automatically at that position
  • Dragging the scrollbar with mouse
  • Resizing the window and checking (for test2)

Please let me know if you need more help.

Thanks.

@DylanBulmer
Copy link
Author

Ah, ok, I can look into that, it might take a little bit since finals week is this week, but I'll make sure that I'll fix it so both tests pass.

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.

2 participants