Small bug fixes and localization of textSetting

SmartThingsCommunity / smartapp-sdk-nodejs

Javascript/NodeJS SDK to create SmartThings SmartApps

https://smartthings.developer.samsung.com/

Apache License 2.0

146 stars 80 forks source link

Small bug fixes and localization of textSetting #127

Closed lukhong closed 4 years ago

lukhong commented 4 years ago

Bug fixes for two issues. https://github.com/SmartThingsCommunity/smartapp-sdk-nodejs/issues/122 https://github.com/SmartThingsCommunity/smartapp-sdk-nodejs/issues/123

Improvement to allow the localization of textSetting https://github.com/SmartThingsCommunity/smartapp-sdk-nodejs/issues/125

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[x] I have read the CONTRIBUTING document
[ ] My code follows the code style of this project (hint: install an xo editor plugin)
[ ] Any required documentation has been added
[ ] I have added tests to cover my changes

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 9de243726be092f68e9a93d6ca0960f789f447df into ea3ed0f8aac5bd0efb0cac85289895d2225dddd0 - view on LGTM.com

new alerts:

1 for Syntax error

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 7984a4eec80a70f3f3c2ec063bf45a533b663648 into ea3ed0f8aac5bd0efb0cac85289895d2225dddd0 - view on LGTM.com

new alerts:

1 for Syntax error

rossiam commented 4 years ago

It looks like you have a new dependency on the underscore library but no changes to package.json and package-lock.json. Be sure to do a --save when you npm install (i.e. npm install --save underscore and then commit package.json and package-lock.json.

rossiam commented 4 years ago

Don't forget to also include package-lock.json. There were some other commits to that one so you might have to deal with some conflicts.

lukhong commented 4 years ago

@rossiam Thanks for the comments, I updated related .json files. Please review them.

bflorian commented 4 years ago

@rossiam @lukhong this change breaks text fields that aren't required and have no default value. In those cases the value of the field gets set to the default i18n key, for example with a input defined as:

section.textSetting('cameraPatToken').required(false);

Not entering anything results in a value of pages.mainPage.settings.cameraPatToken.defaultValue

lukhong commented 4 years ago

@bflorian Yes, that is true and I tried to find a way to handle it nicely but couldn't. However, I thought we can avoid showing like above by giving an empty string to defaltValue like .defaultValye(''). That is the same way we are already doing to the descrption. How do you think about it?

bflorian commented 4 years ago

@lukhong I think a text field should continue to have a default value only if one is explicitly set. As currently implemented this is a breaking change. I also don't think the standard behavior should be to pass default values through localization, though I can see the value of that in some cases. So how about something like this:

section.textSetting('cameraToken')  // field defaults to blank

section.textSetting('cameraToken')  // field defaults to 'XXXX'
    .defaultValue('XXXX')

section.textSetting('cameraToken')  // default value is translated from the i18n key
    .translatedDefaultValue()