Skip to content

Conversation

@jonahtanjz
Copy link
Contributor

@jonahtanjz jonahtanjz commented Jul 6, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Resolves #1628.

Add String type to Boolean props with default value of true. This is because HTML attribute value is passed as String.
In this case, false will be rendered as String:

<panel header="..." expandable="false">
  ...
</panel>

In this case, minimized will be passed as Boolean:

<panel header="..." minimized>
  ...
</panel>

Anything you'd like to highlight / discuss:

Should we add String type to all Boolean props that are from HTML attributes? Currently I only see the need for props with default value of true as the only way to set it to false as a HTML attribute is to pass a "false" value.
However, if a user input this,

<panel header="..." minimized="true">
  ...
</panel>

minimized will be rendered as String. Not sure if the above example is the correct usage of HTML in general. Should we leave this to the user such that if they want an attribute to be true, just include the attribute there without any value? The above example of passing a true value will trigger warning unless we include String type for all Boolean props coming from HTML.

Testing instructions:
Follow steps in #1628.

Proposed commit message: (wrap lines at 72 characters)
Fix warning for Boolean props


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jul 8, 2021

Should we leave this to the user such that if they want an attribute to be true, just include the attribute there without any value?

Agree with this. Going forward we should (have been for while already though) stick with this for new component props. It should be simpler for both the user and implementation wise.

For the previous existing attributes using this behaviour, it is a breaking change, so we can slate that for v4.
In the meantime I think the docs need some updating on true / false versus "just put the property name there" though =X

Should we add String type to all Boolean props that are from HTML attributes?

It should rightly be added for all props using the toBoolean function (a relic from vue v1->v2 migration), but I think its ok to fix-this-as-we-go to minimise possible side effects, the warning from vue is not critical in comparison. But if you're up for investigating each usage let's do that 😀

@jonahtanjz
Copy link
Contributor Author

Thanks for the review @ang-zeyu

In the meantime I think the docs need some updating on true / false versus "just put the property name there" though

Yup agree. Will open another issue and PR for this :)

It should rightly be added for all props using the toBoolean function (a relic from vue v1->v2 migration), but I think its ok to fix-this-as-we-go to minimise possible side effects, the warning from vue is not critical in comparison. But if you're up for investigating each usage let's do that

Ok got it! Let's go with this 👍

@jonahtanjz
Copy link
Contributor Author

@ang-zeyu I have updated the PR to include String to all relevant props using toBoolean.

@jonahtanjz jonahtanjz changed the title Fix warning for boolean props in panel Fix warning for boolean props Jul 10, 2021
Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Lgtm!

props: {
disabled: {
type: Boolean,
type: [Boolean, String],
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ang-zeyu ang-zeyu added this to the v3.0.5 milestone Jul 17, 2021
@ang-zeyu ang-zeyu merged commit cb1cb36 into MarkBind:master Jul 17, 2021
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.

Vue warning for boolean variables

2 participants