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

Boolean properties should not default to true (e.g. allDaySlot) #15

Closed
matgnt opened this issue Aug 17, 2017 · 4 comments
Closed

Boolean properties should not default to true (e.g. allDaySlot) #15

matgnt opened this issue Aug 17, 2017 · 4 comments

Comments

@matgnt
Copy link
Contributor

matgnt commented Aug 17, 2017

Hi Mosbahi,

thanks for publishing this component! I just found this minor issue with "true" default values of Boolean properties. I figured out with "allDaySlot" which I couldn't disable through setting the component attribute. I think there are 2 options to fix this:

  1. Set the default values of Boolean properties to "false"
    (Issue with this fix: It would change the behavior after an update of this component)
  2. Or set the type of those properties to "Object"

The Polymer documentation says: "Boolean. Results in a non-valued attribute to be either set (true) or removed (false)."
Which is not very precise I think. I found a better (but older) explanation here:
Polymer/polymer#1812 (comment)

Shall I create a pull request?

Matthias

@HaithemMosbahi
Copy link
Owner

Hello,

Thank you for raising this issue. Yes, we have a problem here with "true" default values. When I started wrapping the fullCalendar I wanted to stay on the same page with the default values of the library.

I will probably go with the first option which is the custom elements spec / polymer way of dealing with boolean values and thus it's up to the developer to add / remove boolean attributes.

Fortunately, changing the default values of the existing boolean properties to false will not change the behavior of the component.

Yes, you can create a pull request. Your contribution is welcome 😃

@matgnt
Copy link
Contributor Author

matgnt commented Aug 18, 2017

Great! I'll make the changes.
What I meant with the behavior of the component was the following example:

A developer uses the component without the "all-day-slot" attribute and up until now, he can see the day slot in the calendar, because the default is true. When I change it to false now, it's not visible anymore until he adds this attribute to his code. Which means a code change is necessary with this update to keep the same look and feel as before. I'm not sure how many developers would be affected by that.

What do you think?

@HaithemMosbahi
Copy link
Owner

Yes I agree with you. However, I think both options may require some changes in the developer's code. Even with the Object type, the developer has to change all boolean attributes to meet the object syntax ( all-day-slot="true" ).

We will release a new version and we will mention all these changes in the documentation.

Sent from my Huawei ALE-L21 using FastHub

matgnt added a commit to matgnt/scheduler-component that referenced this issue Aug 18, 2017
…bahi#15

Component attributes are "true" if set and "false" if not.
Therefore, default values of "true" can never be set to "false" through
attributes.
Explained more in detail here:
Polymer/polymer#1812 (comment)
HaithemMosbahi added a commit that referenced this issue Aug 18, 2017
Fix Boolean default values to "false" as reported in issue #15
@HaithemMosbahi
Copy link
Owner

This issue has been fixed on version 1.4.0

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

No branches or pull requests

2 participants