JoshDSommer / nativescript-slides

A NativeScript plugin that is for Intro Tutorials, Image Carousels or any other slide functionality
Other
70 stars 32 forks source link

Boolean properties set via XML being read as strings #105

Closed toddanglin closed 7 years ago

toddanglin commented 7 years ago

I'm not sure if this is a new issue in TNS 3, or if I've just overlooked it in the past.

I'm observing that boolean properties like pageIndicators are not being handled correctly when the plugin renders because they are being read at runtime as strings. So if I have this configuration:

<Slides:SlideContainer id="slides" row="0" pageIndicators="false" ...

The page indicators will STILL be displayed because the plugin's internal runtime code sees this:

if (this.pageIndicators) { // Always true if ANY value set in XML config b/c string is not undefined
   // Render indicators
}

I think the solution will require converting the XML string property values to booleans. Not sure how other plugins are handling this, but maybe there is a good example to follow. I'll try a workaround locally, but let me know if you'd prefer to have me create a PR. You might want to do some additional testing locally to see if you can reproduce what I'm seeing using the demo project and TNS 3.

toddanglin commented 7 years ago

The "solution" which solves the problem in my local test is to simply add this runtime type check to the setters for any public boolean properties:

set pageIndicators(value: boolean) {
   if (typeof value === 'string') {
      value = (value == 'true');
   }
   this._pageIndicators = value;
}

With this small check, a string will be converted to a boolean type, and everything goes back to working as expected. The fix must be applied to the pageIndicators, loop, disablePan and angular properties (I think).

Does that make sense? Still not sure what triggered this regression...or why other plugins aren't having this same problem (as far as I know)...

JoshDSommer commented 7 years ago

@toddanglin This sounds good, Could this be something new since 3.0 or maybe it just always worked this way and nobody really noticed until now since page indicators are a two step process, turn them on then style with css. they've just always been on, but nobody notices this bug since they were not styling them.

toddanglin commented 7 years ago

Feels like something new. I've got several Sliders in an app that have a mix of disablePan and pageIndicators turned on/off. I think it has always worked in the past. I think.

I tested today with TNS 3.0.1 and TNS 3.1.0 and the problem exists in both of those runtimes (on iOS, at least). Haven't tested with 2.5.x to see if the problem is gone there. I suspect this might be related to TNS 3, but need to verify.

In any case, this solution Works on My Machine™. :) Validate it on your end and perhaps worth merging-in.

JoshDSommer commented 7 years ago

Sounds good! thanks for all the feedback I appreciate it 🍻

toddanglin commented 7 years ago

@EddyVerbruggen Have you encountered anything like this in your plugins?

JoshDSommer commented 7 years ago

fixed in this release https://github.com/TheOriginalJosh/nativescript-slides/releases/tag/2.2.10