-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: {{phrase-editor}} amends #5991
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,67 @@ | ||
import Component from '@ember/component'; | ||
import { get, set } from '@ember/object'; | ||
import { inject as service } from '@ember/service'; | ||
|
||
export default Component.extend({ | ||
dom: service('dom'), | ||
classNames: ['phrase-editor'], | ||
item: '', | ||
remove: function(index, e) { | ||
this.items.removeAt(index, 1); | ||
this.onchange(e); | ||
didInsertElement: function() { | ||
this._super(...arguments); | ||
// TODO: use {{ref}} | ||
this.input = get(this, 'dom').element('input', this.element); | ||
}, | ||
add: function(e) { | ||
const value = get(this, 'item').trim(); | ||
if (value !== '') { | ||
set(this, 'item', ''); | ||
const currentItems = get(this, 'items') || []; | ||
const items = new Set(currentItems).add(value); | ||
if (items.size > currentItems.length) { | ||
set(this, 'items', [...items]); | ||
this.onchange(e); | ||
onchange: function(e) {}, | ||
search: function(e) { | ||
// TODO: Temporarily continue supporting `searchable` | ||
let searchable = get(this, 'searchable'); | ||
if (searchable) { | ||
if (!Array.isArray(searchable)) { | ||
searchable = [searchable]; | ||
} | ||
searchable.forEach(item => { | ||
item.search(get(this, 'value')); | ||
}); | ||
} | ||
this.onchange(e); | ||
}, | ||
onkeydown: function(e) { | ||
switch (e.keyCode) { | ||
case 8: | ||
if (e.target.value == '' && this.items.length > 0) { | ||
this.remove(this.items.length - 1); | ||
oninput: function(e) {}, | ||
onkeydown: function(e) {}, | ||
actions: { | ||
keydown: function(e) { | ||
switch (e.keyCode) { | ||
case 8: // backspace | ||
if (e.target.value == '' && get(this, 'value').length > 0) { | ||
this.actions.remove.bind(this)(get(this, 'value').length - 1); | ||
} | ||
break; | ||
case 27: // escape | ||
set(this, 'value', []); | ||
this.search({ target: this }); | ||
break; | ||
} | ||
this.onkeydown({ target: this }); | ||
}, | ||
input: function(e) { | ||
set(this, 'item', e.target.value); | ||
this.oninput({ target: this }); | ||
}, | ||
remove: function(index, e) { | ||
get(this, 'value').removeAt(index, 1); | ||
this.search({ target: this }); | ||
this.input.focus(); | ||
}, | ||
add: function(e) { | ||
const item = get(this, 'item').trim(); | ||
if (item !== '') { | ||
set(this, 'item', ''); | ||
const currentItems = get(this, 'value') || []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get the desire to align with DOM APIs here, but I personally don't like that the singular value is being used to represent a collection. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agree here, I almost I'll have another look today though. |
||
const items = new Set(currentItems).add(item); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (thinking out loud) Creating and throwing away a Set every time you add an item seems unfortunate, but considering it's public API, doing something where the Set is an internal implementation with an Array as the public API seems tricky when combined with the typical data-down approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was just for unique diff checking. Me personally, I'm not too worried about creating Sets, it's just a native object - I'm not sure if this is majorly any different from creating and throwing away Arrays? The alternative would be loops and conditionals, Set seems to be the more appropriate tool There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe I'm worried over nothing. To clarify, I love the use of Set here for uniquing. It's just the throwing it away on every add that raises a (probably irrelevant) flag in my head. |
||
if (items.size > currentItems.length) { | ||
set(this, 'value', [...items]); | ||
this.search({ target: this }); | ||
} | ||
break; | ||
} | ||
}, | ||
oninput: function(e) { | ||
set(this, 'item', e.target.value); | ||
}, | ||
onchange: function(e) { | ||
let searchable = get(this, 'searchable'); | ||
if (!Array.isArray(searchable)) { | ||
searchable = [searchable]; | ||
} | ||
searchable.forEach(item => { | ||
item.search(get(this, 'items')); | ||
}); | ||
} | ||
}, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,4 +8,5 @@ | |
padding: 0; | ||
height: 10px; | ||
margin-right: 3px; | ||
font-size: 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems out of place. What's the story here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah, I almost didn't put this in here (see the P.S. in the PR description). It was just easier to fit in here, plus this PR is all about 'phrase-editor amends', which the above fits into. Basically I was trying to keep the 'Remove' text that is inside the little delete buttons for styling/a11y reasons, it got removed in a previous PR without me realizing and 'Remove' text was then visible. This just rehides it. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
<ul> | ||
{{#each items as |item index|}} | ||
<li> | ||
<button type="button" onclick={{action remove index}}>Remove</button>{{item}} | ||
</li> | ||
{{/each}} | ||
</ul> | ||
<label class="type-search"> | ||
<ul> | ||
{{#each value as |item index|}} | ||
<li> | ||
<button type="button" {{action 'remove' index}}>Remove</button>{{item}} | ||
</li> | ||
{{/each}} | ||
</ul> | ||
<label class="type-search"> | ||
<span>Search</span> | ||
<input onchange={{action add}} onsearch={{action add}} oninput={{action oninput}} onkeydown={{action onkeydown}} placeholder="{{placeholder}}" value="{{item}}" type="search" name="s" autofocus="autofocus" /> | ||
</label> | ||
<input onchange={{action 'add'}} onsearch={{action 'add'}} oninput={{action 'input'}} onkeydown={{action 'keydown'}} placeholder={{if (eq value.length 0) placeholder}} value={{item}} type="search" name="s" autofocus="autofocus" /> | ||
</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should either be
this.send('remove', get(this, 'value').length - 1)
orremove
should be pulled out of actions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading up on
send
it seems to be used for bubbling messages up through multiple components and/or controllers and routes.That's not what I want to do here.
I want to call a function on the same component, that for 'reasons' live on a different object within the same component.
As they live on a different object, and ember does its magical ✨ rebind when you use them via
{{action}}
,this
within the function doesn't mean what one thinks it means.bind
is the standard way of settingthis
to whatever you want it to be. So I use that to setthis
to what it is when you use{{action}}
. No bubbling, no removing the action and not realizing that I have a similarly named action on the controller and things still 'working' but in the wrong way without me realizing (which I believe could be a footgun withsend
?)Why not pull it out of actions? As of ember 2.18
actions
are the way of saying 'hey template 👋 , these are my public API methods, I have 3 dragons hiding in here so please don't use anything else!' 😄 So right now I generally favour them (as of 2.18), even though (I believe?) theactions
object is being removed.sendAction
has also been deprecated and I have a feelingsend
will probably go the same way soon considering embers tendency lately to favour a more standard javascript approach, and the speed at which ember is trying to deprecate things lately, I generally feel its safer to use standard javascript where possible (I think the majority of my app is deprecated or soon to be deprecated at this point 😅 ).So they are my reasons for using standard
bind
, but if you still feel strongly that it needs changing then I'm happy to do so.I suppose the question is, does
send
give me anything extra here that I need apart from it just being 'the ember 2.18 way'? In the same way thatactions
is 'the ember 2.18 way', but it also gives me a pseudo public interface as an extra benefit.Anyway I'm going to see if I can add some testing, so I'll hold off on a merge here, lets catch up later.
P.S. Yes I am watching GoT 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing since
send
in controllers/routes is different thansend
in components, butsend
in components doesn't bubble out. You needsendAction
for that. All thatsend
does in a component is make it so you don't have to do the bind dance.Beyond looking nicer and not creating a local bound function, by using the public API, if this changes in the future (e.g., actions deprecated) you can expect a codemod to work.
Here's a minimal example from Nomad: https://github.com/hashicorp/nomad/blob/master/ui/app/components/two-step-button.js#L45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
Thanks for the extra opinion here. I read up a little more on this:
Component::send
https://api.emberjs.com/ember/3.10/classes/Component/methods/send?anchor=send
Controller::send
https://api.emberjs.com/ember/3.10/classes/Controller/methods/send?anchor=send
Both of the above documentation pages use exactly the same documentation, and both link to exactly the same source code, they both bubble out:
https://github.com/emberjs/ember.js/tree/v3.10.2/packages/@ember/-internals/runtime/lib/mixins/action_handler.js#L167
So as far as I understand they both do exactly the same thing? The thing they add over
bind
is mainly the potential bubbling.https://github.com/emberjs/ember.js/blob/41f1657f68735f214efeaa96eac96e8e093b4981/packages/%40ember/-internals/runtime/lib/mixins/action_handler.js#L199-L220
So, looking at the above code what does
send
give me extra? i.e. why should I use it?We can see that
send
basically bindsthis
to the actions object:There is a defensive assertion to check for the object having been destroyed, which I believe gets compiled out? Plus this check is only useful if you have asynchronicity involved, which we don't here (and I'm avoiding in all my components or using events to organize). And then of course it adds the bubbling functionality, which I don't need here.
I've actually just realized the sideeffect-y
target
property on the object also, so if I was to usesend
I'd have to remember not to use/be careful using thattarget
property myself in the future (it essentially should beprivate
). I know of at least one place where I've used this already without knowing that its used for other things:consul/ui-v2/app/components/event-source.js
Line 35 in 553d3cc
Before reading the above I was all for switching, but it's confusing enough to be not immediately clear what it does in either components/routes/controllers. I don't mind writing
bind
, it looks more javascript-y to me and potentially to other ember and non-ember folks, most javascript devs will know whatbind
does but possibly notsend
.send
does exactly the same, but adds things I don't want/need, and could potentially cause confusing sideeffect-y problems that wouldn't immediately make sense unless I'd done this little investigation. This final thing is the thing that worries me the most as I could have quite easily walked into thetarget
problem by accident by usingsend
overbind
. It's gone from a thing that 'probably doesn't matter either way', to 'potentially could cause issues'.Lastly, thanks for the discussion here, I really do appreciate it and it leads to this super interesting insights into ember itself. Lots of TIL 🙌
Thanks,
John
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the
target
detail is one of a handful of sneaky keys you aren't allowed to use (some blow up more spectacularly than others). I really wish all user-land props were in their own object, but hindsight 20/20 I guess.Just for further clarification/internals details...
Bubbling will only happen if
target
is set and the action being sent returnstrue
. Target, although possible to be set by a user, is generally set by the framework.Target is never set by the framework on components. Target is always set by the framework on Routes and Controllers during instantiation, to create the chain of bubbling events.
So aside from the possible unknowing setting of
target
in user-land, components will never bubble onsend
. That's whatsendAction
is for.If you're still in a spelunking mood, the Controller/Route
create
call happens here but it's some gnarly code that isn't very easy to follow.