From 44253a166ad12b7cd538eb818a5a1aa82fe5aec7 Mon Sep 17 00:00:00 2001 From: Chris Parry Date: Tue, 3 Apr 2018 10:07:42 +0100 Subject: [PATCH 1/5] Component#isVisoble deprecation first draft --- text/0000-deprecate-component-isvisible.md | 58 ++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 text/0000-deprecate-component-isvisible.md diff --git a/text/0000-deprecate-component-isvisible.md b/text/0000-deprecate-component-isvisible.md new file mode 100644 index 00000000000..0c48daed118 --- /dev/null +++ b/text/0000-deprecate-component-isvisible.md @@ -0,0 +1,58 @@ +- Start Date: 2018-03-28 +- RFC PR: +- Ember Issue: + +# Summary + +The aim of this RFC is to deprecate the component `isVisible` attribute +is not used by ember internally and left undefined unless manually set. +It's poorly documented and component visibility it better managed in +template space rather than JS. + +# Motivation + +Component visbility is better handled in tempalte space +apposed to the JS alternative. Using this attribute to toggle component visibility +introduces bugs with `StyleBindingReference` not updating. Currently, it adds +`display: none` as an inline style which removes all other inline +styles attached to an element. It is an additional way of hiding components +that with its removal will reduce confusion on which approach +to take when performing said functionality. See transition path for available +options. + +# Transition Path + +In cases where the `isVisible` property is used we provide a deprecation warning +with a link to the deprecation guide which states why it was deprecated and/or the +options available to hide the component. + +There are several options available to hiding elements +such as ``(hidden is valid for all elements +and is semantically correct) or wrapping the component in a template +conditional `{{#if}}`statement which do not interfere with +the `StyleBindingReference`. Components `classNames` an `classNameBindings` +could also be used to add hidden classes. + +# How We Teach This + +The `isVisible` attribute is rarely used so deprecating in a future blog post +would be sufficient. It will need to be removed from the API docs. It would be +beneficial to add documentation on hiding components to the Ember guides with +suggested ways of hiding components with a simple example. + +# Drawbacks + +Apps that are using this feature will have to decide on an appropriate upgrade +path suited for them. + +# Alternatives + +An alternative option would be to change the `isVisible` property to use the +hidden attribute of the element instead of adding the inline style. + +# Unresolved questions + +What would be the suggested way of hiding components from the DOM in Ember as +there are a few options to choose from? + +When will support for the deprecated property end? From bbecb441b73d5884172d94872ea5f6aea12c39c8 Mon Sep 17 00:00:00 2001 From: Simon Ihmig Date: Tue, 3 Apr 2018 11:57:59 +0200 Subject: [PATCH 2/5] Fix some typos First, thanks for putting this together! This fixes a few small typos, and rephrases "attribute" to "property", as the first is commonly used when creating HTML nodes. See https://stackoverflow.com/questions/6003819/what-is-the-difference-between-properties-and-attributes-in-html --- text/0000-deprecate-component-isvisible.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/text/0000-deprecate-component-isvisible.md b/text/0000-deprecate-component-isvisible.md index 0c48daed118..f60da593b8b 100644 --- a/text/0000-deprecate-component-isvisible.md +++ b/text/0000-deprecate-component-isvisible.md @@ -4,15 +4,15 @@ # Summary -The aim of this RFC is to deprecate the component `isVisible` attribute -is not used by ember internally and left undefined unless manually set. +The aim of this RFC is to deprecate the component's `isVisible` property. +It is not used by Ember internally and left undefined unless manually set. It's poorly documented and component visibility it better managed in template space rather than JS. # Motivation -Component visbility is better handled in tempalte space -apposed to the JS alternative. Using this attribute to toggle component visibility +Component visbility is better handled in template space +apposed to the JS alternative. Using this property to toggle component visibility introduces bugs with `StyleBindingReference` not updating. Currently, it adds `display: none` as an inline style which removes all other inline styles attached to an element. It is an additional way of hiding components @@ -35,7 +35,7 @@ could also be used to add hidden classes. # How We Teach This -The `isVisible` attribute is rarely used so deprecating in a future blog post +The `isVisible` property is rarely used so deprecating in a future blog post would be sufficient. It will need to be removed from the API docs. It would be beneficial to add documentation on hiding components to the Ember guides with suggested ways of hiding components with a simple example. From e9ad24479768c3287d094a014060b32b5dd45874 Mon Sep 17 00:00:00 2001 From: Chris Parry Date: Tue, 3 Apr 2018 10:07:42 +0100 Subject: [PATCH 3/5] Component#isVisible deprecation first draft Updated Support --- text/0000-deprecate-component-isvisible.md | 57 ++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 text/0000-deprecate-component-isvisible.md diff --git a/text/0000-deprecate-component-isvisible.md b/text/0000-deprecate-component-isvisible.md new file mode 100644 index 00000000000..f668244188c --- /dev/null +++ b/text/0000-deprecate-component-isvisible.md @@ -0,0 +1,57 @@ +- Start Date: 2018-03-28 +- RFC PR: +- Ember Issue: + +# Summary + +The aim of this RFC is to deprecate the component `isVisible` attribute +is not used by ember internally and left undefined unless manually set. +It's poorly documented and component visibility it better managed in +template space rather than JS. + +# Motivation + +Component visbility is better handled in tempalte space +apposed to the JS alternative. Using this attribute to toggle component visibility +introduces bugs with `StyleBindingReference` not updating. Currently, it adds +`display: none` as an inline style which removes all other inline +styles attached to an element. It is an additional way of hiding components +that with its removal will reduce confusion on which approach +to take when performing said functionality. See transition path for available +options. + +# Transition Path + +In cases where the `isVisible` property is used we provide a deprecation warning +with a link to the deprecation guide which states why it was deprecated and/or the +options available to hide the component. As it is a public API it will be removed +in the next major version release (4.0). + +There are several options available to hiding elements +such as ``(hidden is valid for all elements +and is semantically correct) or wrapping the component in a template +conditional `{{#if}}`statement which do not interfere with +the `StyleBindingReference`. Components `classNames` an `classNameBindings` +could also be used to add hidden classes. + +# How We Teach This + +The `isVisible` attribute is rarely used so deprecating in a future blog post +would be sufficient. It will need to be removed from the API docs. It would be +beneficial to add documentation on hiding components to the Ember guides with +suggested ways of hiding components with a simple example. + +# Drawbacks + +Apps that are using this feature will have to decide on an appropriate upgrade +path suited for them. + +# Alternatives + +An alternative option would be to change the `isVisible` property to use the +hidden attribute of the element instead of adding the inline style. + +# Unresolved questions + +What would be the suggested way of hiding components from the DOM in Ember as +there are a few options to choose from? From a508fe70f065c78344c011e5e7f17a690e3bf2ee Mon Sep 17 00:00:00 2001 From: Chris Parry Date: Tue, 24 Apr 2018 08:35:32 +0100 Subject: [PATCH 4/5] Added some suggested changes. --- text/0000-deprecate-component-isvisible.md | 38 +++++++++------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/text/0000-deprecate-component-isvisible.md b/text/0000-deprecate-component-isvisible.md index f668244188c..07439ebadb8 100644 --- a/text/0000-deprecate-component-isvisible.md +++ b/text/0000-deprecate-component-isvisible.md @@ -11,14 +11,13 @@ template space rather than JS. # Motivation -Component visbility is better handled in tempalte space -apposed to the JS alternative. Using this attribute to toggle component visibility -introduces bugs with `StyleBindingReference` not updating. Currently, it adds -`display: none` as an inline style which removes all other inline -styles attached to an element. It is an additional way of hiding components -that with its removal will reduce confusion on which approach -to take when performing said functionality. See transition path for available -options. +`isVisible` is super legacy and not entirely necessary Component visbility +is better handled in tempalte space apposed to the JS alternative. Using +this attribute to toggle component visibility introduces bugs with `StyleBindingReference` +not updating. Currently, it adds `display: none` as an inline style which removes +all other inline styles attached to an element. It is an additional way of +hiding components that with its removal will reduce confusion on which approach +to take when performing said functionality. # Transition Path @@ -30,7 +29,7 @@ in the next major version release (4.0). There are several options available to hiding elements such as ``(hidden is valid for all elements and is semantically correct) or wrapping the component in a template -conditional `{{#if}}`statement which do not interfere with +conditional `{{#if}}` statement which do not interfere with the `StyleBindingReference`. Components `classNames` an `classNameBindings` could also be used to add hidden classes. @@ -38,20 +37,15 @@ could also be used to add hidden classes. The `isVisible` attribute is rarely used so deprecating in a future blog post would be sufficient. It will need to be removed from the API docs. It would be -beneficial to add documentation on hiding components to the Ember guides with -suggested ways of hiding components with a simple example. +beneficial to add documentation on hiding components to the Ember guides with the +conditional handlebar helper. +`{{#if showComponent}}` + `{{component}}` +`{{/if}}` -# Drawbacks - -Apps that are using this feature will have to decide on an appropriate upgrade -path suited for them. +Alternatively, with the now widely supported HTML hidden attribute using a simple +`` where isHidden can be toggled. # Alternatives -An alternative option would be to change the `isVisible` property to use the -hidden attribute of the element instead of adding the inline style. - -# Unresolved questions - -What would be the suggested way of hiding components from the DOM in Ember as -there are a few options to choose from? +An alternative option would be to to keep `isVisible`. From 5bd04ccbdb4be7c4b3aff2e56cbdd7a49d3fb7cf Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 24 Apr 2018 10:07:55 -0400 Subject: [PATCH 5/5] Small copy edits... --- text/0000-deprecate-component-isvisible.md | 54 +++++++++++++--------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/text/0000-deprecate-component-isvisible.md b/text/0000-deprecate-component-isvisible.md index df8d61436e6..3e8c90856c3 100644 --- a/text/0000-deprecate-component-isvisible.md +++ b/text/0000-deprecate-component-isvisible.md @@ -11,40 +11,50 @@ template space rather than JS. # Motivation -`isVisible` is super legacy and not entirely necessary Component visbility -is better handled in tempalte space apposed to the JS alternative. Using -this attribute to toggle component visibility introduces bugs with `StyleBindingReference` -not updating. Currently, it adds `display: none` as an inline style which removes -all other inline styles attached to an element. It is an additional way of -hiding components that with its removal will reduce confusion on which approach -to take when performing said functionality. +Setting the isVisible property on a component instance as a way to toggle +the visibility of the component is confusing. The majority of its usage +predates even Ember 1.0.0, and modern Ember applications already completely +avoid using isVisible in favor of simpler conditionals in the template +space. + +In addition, when `isVisible` is used today it often introduces subtle (and +difficult to track down) bugs due to its interaction with the `style` +attribute (toggling `isVisible` clobbers any existing content in `style`). + +Simply put, removing `isVisible` will reduce confusion amongst users. # Transition Path -In cases where the `isVisible` property is used we provide a deprecation warning -with a link to the deprecation guide which states why it was deprecated and/or the -options available to hide the component. As it is a public API it will be removed -in the next major version release (4.0). +Whenever `isVisible` is used a deprecation will be issued with a link to +the deprecation guide explaining the deprecation and how to refactor in order +to avoid it. + +Given that `Component#isVisible` is a public API, deprecating now would +schedule for removal in the next major version release (4.0). There are several options available to hiding elements such as ``(hidden is valid for all elements and is semantically correct) or wrapping the component in a template -conditional `{{#if}}` statement which do not interfere with -the `StyleBindingReference`. Components `classNames` an `classNameBindings` +conditional `{{#if}}` statement. Components `classNames` and `classNameBindings` could also be used to add hidden classes. # How We Teach This -The `isVisible` property is rarely used so deprecating in a future blog post -would be sufficient. It will need to be removed from the API docs. It would be -beneficial to add documentation on hiding components to the Ember guides with the -conditional handlebar helper. -`{{#if showComponent}}` - `{{component}}` -`{{/if}}` +The `isVisible` property is rarely used, the deprecation along with a mention +in a future blog post would be sufficient. + +We should consider adding documentation on hiding components to the Ember +guides with the conditional handlebar helper or via the widely supported `hidden` +attribute. + +```hbs +{{#if showComponent}} + {{component}} +{{/if}} -Alternatively, with the now widely supported HTML hidden attribute using a simple -`` where isHidden can be toggled. +{{! or }} + +``` # Alternatives