search

SmaLLAlien / epam_fe_2019_belousov

FE 2019
Apache License 2.0
0 stars 0 forks source link

HW22 #22

Open zheleznov opened 4 years ago

zheleznov commented 4 years ago

Hi Vlad.

Not bad with this task, but some comments

  1. Settings to jQuery plugins usually passed as an options object. Also it can have default options, which we can override by passing our own jQery plugin options

For example for modal on timeout - It's will be better to implement this logic by passing additional options to the plugin, which can set it behaviour, instead of setting timeout directly on the page and call modal after 10 seconds. Something like this plugin

It's also can get other options which will configure the plugin

  1. In this case you can use chaining to prevent calling the same DOM node again chaining

  2. this.eventsHandler(); better to call this something like setEventHandlers. To describe what the func do

  3. ALso chainings chainings

  4. Adding overflow hidden to body during modal initialization leads to hiding vertical scroll bar and as a result small content blinking. Better to avoid such things

  5. Instead of searching for buttons in whole DOM you can look for them just in your wrapperModal via jQuery.find or use delegation buttons

SmaLLAlien commented 4 years ago

Hi, Roman! thank you for issue. I fixed bugs (hope without creating new ones:) ) deleted old pull request and created new one. Am I allowed to finish this pull request ? Thanks )