Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ui5-checkbox): Add ui5-checkbox components default values #408

Merged
merged 3 commits into from
May 20, 2019

Conversation

gustavokath
Copy link
Contributor

  • Add default value as false for checked, disabled, read-only, wrap

Fixes: #407

 - Add default value as false for cheked, disabled, read-only, wrap
const checkBox = browser.findElementDeep("#cb1");

assert.strictEqual(checkBox.checked, false, "Check if default value for checked is false");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try with checkBox.getProperty("checked").
The checkbox is "wdio" (the test framework in use) wrapper for the elements in the html page .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😄

@ilhan007
Copy link
Member

Hello @gustavokath ,
thanks for reporting.

Yes, the boolean properties should return false by default, not undefined.

Actually, we are working on other change, not related to this issue, but addresses the issue with the boolean (and string) default values.
Now, as you created an issue and pull request, I decided to make a separate pull request, so the boolean properties default should not be an issue (#409) any more.

With this pull request, all components` boolean properties should return false by default
and the developers would not have to declare defaultValue: false in the metadata, although there is such option (and you fixed the the ui5-checkbox boolean props this way).

Furthermore, all boolean properties of the ui5 web components should be "false" by default, the can`t be "true" by default, so declaring defaultValue: false becomes redundant.

However,
I will accept the pull request, as it also updates the JSDoc, but once the tests are green.
About the failing test, look at the inline comment.

best,
ilhan

@gustavokath
Copy link
Contributor Author

Hi @ilhan007,
Thanks and the build is passing now, thanks for the suggestion.

Also, this change you've done in your PR will help us that are using the ui5-webcomponents. I agree with you that having a place where all boolean values are set as default to false is a much better approach than go over all components and update them.

Thanks

@ilhan007 ilhan007 merged commit 9bdd2c5 into SAP:master May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui5-checkbox has no default value for booleans
2 participants