Potential / Infrequent Issues

[mlisook]

Let me say first that these do not affect my use case - I'm happy with the element as is. They could be issues in other use cases. Feel free to close this if you don't feel they are important or likely to affect real world use.

• scrollLeft is truncated to a whole number (effectively Math.floor()) at least in chromium based browsers, while getBoundingClientRect().width is not. This can lead to a situation where this.container.scrollLeft >= (this._numberOfImages-1) * this._getRect().width evaluates to something like 375 >= 375.75 (container is 501px, 4 images, last image). One could imagine fractions creeping in also if the width of the control is set by percentages also.

• _computeNumberOfImages() is only called in the connectedCallback(). This can be a potential problem if the images are populated by <template is="dom-repeat" ...> and the array is retrieved via ajax such as a fetch. In that case the template could be rendered after connectedCallback() and the _numberOfImages property will remain at zero - dots will not appear, right/left navigation will not hide/display correctly.

• Depending on the content, it might be desirable to have a default z-index on .right-icon and .left-icon. Of course, if needed, that can be set in --left-navigation-icon-mixin or --right-navigation-icon-mixin.

• getBoundingClientRect() triggers a repaint and it can be called multiple times during an image change. It's not that bad since it is in response to a user action or the timer on auto scroll, but it could be optimized by just calling it once and saving the result.

Again, these do not affect my use case, but I can cause them if I am deliberate.

Thanks for publishing this.

[a1626]

@mlisook i really appreciate the feedback and the fact that you liked my element. There's really lot of new information in it for me.

Now to get to your feedback

• ScrollLeft: This is a really good corner case (Something i don't think i could have thought of). I would really love to test this case and will fix it.
• _computeNumberOfImages(): This is something even i realized while working on test cases for the element. I think i'll fix it in future release and move it to some observer.
• z-index: I don't think i'll cover this case. i don't this will be required in most of the cases (Please correct me if i'm wrong). As you pointed out user can easily cover this by using mixin.
• getBoundingClientRect: It's a really good idea but, i think there can be one issue. What if user changes the width (RWD, is one genuine case i can think of)?

[a1626]

I've fixed point number 2 (dynamically change images) and now it is possible in v2.0.0