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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 21, 2019
Merged

ui: {{phrase-editor}} amends #5991

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
535dff0
ui: {{phrase-editor}} amends
Jun 19, 2019
e69942e
ui: add some phrase-editor integration tests
Jun 20, 2019
a83b727
ui: Add a little more detail to the todo comment
Jun 20, 2019
3ea5fee
ui: remove `get` import which I don't need
Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 54 additions & 31 deletions ui-v2/app/components/phrase-editor.js
View file
Open in desktop
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: ref
Copy link

Choose a reason for hiding this comment

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

What does this TODO mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a reference to {{ref}}

https://octane-guides-preview.emberjs.com/release/components/glimmer-components-dom/#toc_communicating-between-elements-in-a-component

I added a little more detail to the comment in another commit

Copy link

Choose a reason for hiding this comment

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

ooOOOoo, so Reacty. I'm looking forward to that.

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);
Copy link

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) or remove should be pulled out of actions.

Copy link
Contributor Author

@johncowen johncowen Jun 20, 2019 •
edited
Loading

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 setting this to whatever you want it to be. So I use that to set this 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 with send?)

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?) the actions object is being removed.

sendAction has also been deprecated and I have a feeling send 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 that actions 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 😄

Copy link

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 than send in components, but send in components doesn't bubble out. You need sendAction for that. All that send 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

Copy link
Contributor Author

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

send(actionName, ...args) {
    assert(
      `Attempted to call .send() with the action '${actionName}' on the destroyed object '${this}'.`,
      !this.isDestroying && !this.isDestroyed
    );
    if (this.actions && this.actions[actionName]) {
      let shouldBubble = this.actions[actionName].apply(this, args) === true;
      if (!shouldBubble) {
        return;
      }
    }

    let target = get(this, 'target');
    if (target) {
      assert(
        `The \`target\` for ${this} (${target}) does not have a \`send\` method`,
        typeof target.send === 'function'
      );
      target.send(...arguments);
    }
  }

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 binds this to the actions object:

// create a local bound function
this.actions[actionName].apply(this, args)
//or actionName = 'remove';
this.actions[actionName].bind(this)(...args)

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 use send I'd have to remember not to use/be careful using that target property myself in the future (it essentially should be private). 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

set(this, 'target', $previous);

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 what bind does but possibly not send. 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 the target problem by accident by using send over bind. 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

Copy link

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 returns true. 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 on send. That's what sendAction 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.

}
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') || [];
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@johncowen johncowen Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah agree here, I almost split it by \n to make it a string and then joined it back up again on the other side. It was also detail.value at one point using CustomEvent. I think the thing that makes this 'ok' for me is the fact that as I'm using ember/javascript there is nothing to be gained in splitting/joining things apart from absolute parity with DOM APIs, I'm not sure there is anything practical to be gained from it (at least the target.value is recognisable and and therefore clear what is going on)

I'll have another look today though.

const items = new Set(currentItems).add(item);
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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'));
});
}
},
},
});
1 change: 1 addition & 0 deletions ui-v2/app/styles/base/components/pill/layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@
padding: 0;
height: 10px;
margin-right: 3px;
font-size: 0;
Copy link

Choose a reason for hiding this comment

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

This seems out of place. What's the story here?

Copy link
Contributor Author

@johncowen johncowen Jun 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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.

}
20 changes: 10 additions & 10 deletions ui-v2/app/templates/components/phrase-editor.hbs
View file
Open in desktop
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>
7 changes: 6 additions & 1 deletion ui-v2/app/templates/dc/services/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@
{{/block-slot}}
{{#block-slot 'toolbar'}}
{{#if (gt items.length 0) }}
{{#phrase-editor placeholder=(if (eq terms.length 0) 'service:name tag:name status:critical search-term' '') items=terms searchable=searchable}}{{/phrase-editor}}
{{phrase-editor
placeholder='service:name tag:name status:critical search-term'
value=(slice 0 terms.length terms)
onchange=(action (mut terms) value='target.value')
searchable=searchable
}}
{{/if}}
{{/block-slot}}
{{#block-slot 'content'}}
Expand Down