gromo / jquery.scrollbar

jQuery CSS Customizable Scrollbar
GNU General Public License v2.0
756 stars 238 forks source link

Some points need to improve! #41

Closed e-cloud closed 8 years ago

e-cloud commented 9 years ago

I just checked the source at a simple walkthrough, and found something, IMHO, need improvement.

  1. umd support
  2. more angular support
  3. debug property in defaults object is useless
  4. log property in browser object depends on the inner variables debug which is not exposed to the user
  5. some source(this one, this one) use the charactor var. In my view, it's more important that source can be readable rather than short(which can be minify by other tools like uglify)
  6. it there any possibility that write more useful document in the source so that more people(like me:smile:) can participate in improvement
e-cloud commented 9 years ago

i have just made a little effort here #40, hope for any suggestion!

e-cloud commented 9 years ago

one more point: the version number in the src hasn't been updated yet~~

gromo commented 9 years ago

Thank you for your participation.

I am working on new version now with more accurate code (actually I refactor it from scratch), but it will take me a lot of time to research and finish. I hope new version will have readable code with detailed comments. API/options will be extended, but backward compatible.

I was going to use options.debug to be able to debug each scrollbar separately, but didn't make it. I used short-named variables for more readable code - I thought so when I was coding :). Now I see that I was wrong - these variables are hard to read.

I don't have time to review and check your code functionality, but if you are sure it works, I will pull your changes tomorrow morning.

Thank you again for your participation.

Best Regards, Yuriy.

e-cloud commented 9 years ago

wait, i haven't got a full test for the PR. Till this weekend i have enough time to work for it. So, could you wait for some more time?

gromo commented 9 years ago

Of course. Write me when everything will be done and I will pull changes

e-cloud commented 9 years ago

@gromo, I've got some test for my PR, and i works well. I can say that my PR passed my test. What needs illustrating is that i write my test via adding some demo directly to the local gh-pages. So should i make another request for gh-pages?

gromo commented 9 years ago

I think there is no need

e-cloud commented 9 years ago

@gromo , i think it would be better that seperate the angular code. one solution could be two .js file that one has angular support and the other one doesn't. it is more modulize-like and more reasonable

gromo commented 9 years ago

I don't think it is good idea. Plugin is not so huge to separate it, and supporting two different files is more complex. IMHO, better leave it as it is.

sateffen commented 9 years ago

I would prefer the removal of the angular-specific code as well, because in our project for example we don't use angularjs, and every line that can get removed is a good one.

You could setup a build-process with es6-imports, that bundles the code itself, or set up a browserify/webpack build-system, that does the job for you. With that you don't need to maintain 2 files, you just need to maintain 2 parts of your code, and this "problem" would be solved.

Any thoughts about this?

gromo commented 9 years ago

Maybe move angular module/directive into separate file? All who want angular support have to include jquery.scrollbar.js + angular.jquery.scrollbar.js, something like that. Maybe do it this way?

sateffen commented 9 years ago

Even better! Taking the angular directive in a different file allows to write just another file for a knockout-binding for example.

I like the idea

gromo commented 9 years ago

Sorry for delay, have a lot of work these days. I will split files when I will have free time next week.