Placeholder component for power select multiple #817
Conversation
lazybensch
force-pushed
the
placeholder-component-for-power-select-multiple
branch
from
123810e
to
6e5129f
Compare
| @@ -16,8 +16,8 @@ | |||
| {{/if}} | |||
| </li> | |||
| {{else}} | |||
| {{#if (and placeholder (not searchEnabled))}} | |||
| <span class="ember-power-select-placeholder">{{placeholder}}</span> | |||
| {{#if (not-eq placeholder maybePlaceholder)}} | |||
There was a problem hiding this comment.
This is changing the logic and concerns me.
Originally the placeholder only shows if the search was not enabled but if I'm reading this right in here it's going to show even if there is a search input. Am I wrong?
There was a problem hiding this comment.
no you are right, i just realized this myself and am correcting it :) thx for the review
| @@ -17,7 +17,7 @@ | |||
| </li> | |||
| {{else}} | |||
| {{#if (and placeholder (not searchEnabled))}} | |||
| <span class="ember-power-select-placeholder">{{placeholder}}</span> | |||
There was a problem hiding this comment.
One last comment. The way the placeholder component works in the single component is that you render it, and it already check if its placeholder property is there to decide wether it renders anything or not.
In think that you simplify this to
| @@ -70,7 +70,10 @@ export default Component.extend({ | |||
| }), | |||
|
|
|||
| maybePlaceholder: computed('placeholder', 'select.selected.length', function() { | |||
| if (isIE) { | |||
| let component = this.get('placeholderComponent'); | |||
| let hasCustomPlaceholder = component !== 'power-select/placeholder'; | |||
There was a problem hiding this comment.
Since you are not going to render this placeholder if the search is disabled, I think that you don't need this code.
CHANGELOG.md
Outdated
| @@ -6,6 +6,8 @@ | |||
| there before. EPS doens't rely on those attributes and it's unlikely that this will | |||
| break for anyone, but just in case I'll bump a minor version number and keep it in | |||
| beta for some days. | |||
| - [ENHANCEMENT] Providing a custom `placeholderComponent` is available for | |||
There was a problem hiding this comment.
You must put this below # Master.
lazybensch
force-pushed
the
placeholder-component-for-power-select-multiple
branch
from
3b9a183
to
13ad5b5
Compare
|
@lazybensch can you rebase this? The code looks good now, and the tests should be green if you rebase. |
|
@cibernox Hey! Do you know what is the status of this feature? Can it be rebased and merged? Or this was provided at some other later PR's? |
This PR adds support for
placeholderComponent: 'custom-placeholdertopower-select-multiple.I also added a regression test.