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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-implement tag-editing component #5553

Merged
merged 1 commit into from Aug 10, 2015

Conversation

Projects
None yet
5 participants
@acburdine
Member

acburdine commented Jul 14, 2015

refs #3800

This PR re-implements the tag component into Zelda, as well as fixes a number of issues with the old one. The biggest change in this PR is the decision to implement a typeahead library to smooth over the autocomplete code.

While the styles will need to be fixed after this PR, the functionality will exist and work.

Task List:

  • Finish fixing up typeahead workings
  • Hook component up to actual data
  • Fix keyboard shortcuts
  • Fix defocus on tag adding or deleting issue
  • Fix api calls on new post
  • Clean up UI (fix tag deletion)
  • Fix post saving breaking tag saving
  • tests 馃懣
  • cleanup code
  • squash commits
  • eat cake 馃嵃

@acburdine acburdine changed the title from [WIP] Reimplement Tag component to [WIP] Re-implement tag-editing component Jul 14, 2015

@acburdine acburdine force-pushed the acburdine:tag-component branch 3 times, most recently from 1f6ef83 to 0b55607 Jul 16, 2015

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 19, 2015

@ErisDS I added the new /posts/:id/tags endpoints. I split out the updateTags functionality into a utils file but I'm not sure if how I did it is necessarily the best way to do it as it felt like kind of a hack. Would you mind taking a quick look?

@acburdine acburdine force-pushed the acburdine:tag-component branch from 0b55607 to 7b98591 Jul 21, 2015

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 21, 2015

@JohnONolan I was trying to get the input to work similar to pocket.co's tag input, but unfortunately I could not get it to work exactly like it. In the pocket.co version, the input is moved between tags as the user moves left and right with the arrow keys. Because we are using typeahead, doing that is not trivial and results in a lot of computational overhead. However, I did make it so that a user can traverse the current list of tags with the left and right arrow keys, with the currently selected one being highlighted.

Because the user is able to traverse between the tags with the arrow keys, is it okay to remove the click-to-delete-tag functionality for the moment? It will allow me to implement the jqueryui drag-n-drop behavior in a much simpler fashion. It could perhaps be added back in once the css is fixed and the "x" icon is added back in.

@JohnONolan

This comment has been minimized.

Member

JohnONolan commented Jul 22, 2015

Sounds good to me - let's get it in, test it and see how it goes

@halfdan

This comment has been minimized.

Member

halfdan commented Jul 22, 2015

@acburdine Gave this a quick look.

  • Entering , does not finish a tag (you have to press enter)
  • Removing Tags by hitting backspace does not work
  • Tag Input is not reset when switching from an existing post to a new post
  • Tags are not being saved (and strangely enough don't show up in Ember inspector)
@acburdine

This comment has been minimized.

Member

acburdine commented Jul 22, 2015

@halfdan Fixed all that you suggested. As to tags not saving, they are saving for me just fine. The reason they are not showing up in Ember inspector is because the tag input bypasses the ember data store altogether in favor of direct api calls.

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 22, 2015

@ErisDS @jaswilli

There's an issue currently that @halfdan pointed out where the tags are not saving correctly when the post is saved. I tracked this down to the embedded tags relationship in the post model. The issue is that when the tag input component saves the list of tags, the post tags relationship does not get updated in the ember-data store, which results in the tags being overridden by whatever was in the post model on post save.

I could, in theory, update the post model with the new updated tags after the tag input saves them, which would be the simplest option, however that would result in the same list of tags being saved twice, which I would think is unnecessary.

There are a few other ways I can think of to solve this.

  1. Switch from doing a straight API call for the updated tags and instead just update the post model, which would then get saved on post save; however, I'm not sure what this would do regarding binding.
  2. Take the embedded tags out of the post model. This would keep the tags from being fetched on post fetch and saved on post save. I would think this would be the best option, but I'm not sure if the embedded tags are used anywhere else other than the post editor?
  3. Take the ability to save tags out of the PUT /posts/:id/ endpoint. Currently the ability to save tags for a specific post exists in both the PUT /posts/:id/tags/ endpoint and the PUT /posts/:id endpoint. Not sure if this would be a good idea as it seems like kind of a hack to me, and forces api requests to split up saving posts and saving tags for a post.
@ErisDS

This comment has been minimized.

Member

ErisDS commented Jul 22, 2015

There is some discussion about this on the issue here. The major point being I have little-to-no idea what can or can't be done with ember and ember-data, and was looking for guidance from the people in the know to check the API design would work.

The options from an API design perspective are: leave it as it is (PUT /posts/:id/), or move it down a level to use either PUT /posts/:id/tags/ and edit the collection as a whole, or update them individually using POST /posts/:id/tags/ and DELETE /posts/:id/tags/:id.

The major concern is ensuring that this is reliable because bugs like #4999 are the worst kind of bugs.

@@ -305,6 +305,99 @@ posts = {
return deletedObj;
});
},
getTags: function getTags(options) {

This comment has been minimized.

@ErisDS

ErisDS Jul 22, 2015

Member

The naming of these methods needs some consideration. Currently we use consistent 'BREAD' names like api.posts.read(), not 100% sure at this moment how to extend that pattern to include nested relations. I'm thinking it ought to be something along the lines of: api.posts.tags.edit() ?

This comment has been minimized.

@acburdine

acburdine Jul 22, 2015

Member

I would agree

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 22, 2015

Okay. Given that comment thread I would think the best thing to do would be to switch the post model in Ember back to being a traditional async model instead of an embedded record.

@ErisDS

This comment has been minimized.

Member

ErisDS commented Jul 26, 2015

I think that the best way to move forward with this might be to revert back to something closer to the existing version, using the current API endpoint.

The only reason for changing the endpoints was if it helped to make the ember code easier to write and more robust, whereas the change here seems to be causing more problems than it is solving.

Having a robust, working version of the tag component with the same functionality as now (no drag and drop required), which lives inside the PSM and saves safely, is the priority for this issue.

Drag and drop can be added later.

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 26, 2015

Drag and drop is already working minus one function to re-order the tags on drop. Should I go ahead and complete dragndrop or no?

@ErisDS

This comment has been minimized.

Member

ErisDS commented Jul 27, 2015

I would focus on making sure everything else is 100% before looking to complete drag and drop.

@acburdine acburdine force-pushed the acburdine:tag-component branch from 3dd5914 to 9cb8f0d Jul 31, 2015

@acburdine

This comment has been minimized.

Member

acburdine commented Jul 31, 2015

@ErisDS I hooked up the component directly to the post model instead of directly interacting with the API. That way, all data goes through the store and we don't get duplicate saves. That should finish up the main part of this. I will work on tests to get this finished.

@jaswilli / @novaugust / @kevinansfield can you give this a once-over? I may re-order some of the functions in the tag component but all the functionality is there.

@acburdine

This comment has been minimized.

Member

acburdine commented Aug 1, 2015

@ErisDS about half-done with casper tests, should be able to finish those up tomorrow.

@kevinansfield

This comment has been minimized.

Contributor

kevinansfield commented Aug 1, 2015

I haven't had a chance to dig in too deeply yet but at first glance it seems that there may be some tight coupling between the tags input and the post model.

Have you thought about making the input more focused on being a reusable input component and having the post-settings-menu controller deal with the post/tag specifics? Eg, something like:

{{gh-tags-input
    currentTags=post.tags
    availableTags=availableTags
    addTag=(action 'addTag')
    removeTag=(action 'removeTag') }}

If you tried this route were there any issues that you ran into?

@acburdine

This comment has been minimized.

Member

acburdine commented Aug 1, 2015

@kevinansfield I hadn't tried that, but IMO tight coupling between tags input and the post model is necessary in this situation. The primary reason I think is because of situations like #4999, because in the old tag input the tags were bound directly to the post model's tags. Another reason I think is that if it is bound directly to the post tags, and the post autosaves when the user has created new (but mistyped) tags, there is nothing to prevent those new tags from saving with the post. It also keeps the tag-input adding, deleting, and loading functions more cohesive.

It wouldn't be hard to move the code out into the PSM controller, but I don't personally think it's necessary. If we need a re-usable input component in the future I can always abstract the necessary stuff out into a mixin.

@kevinansfield

This comment has been minimized.

Contributor

kevinansfield commented Aug 1, 2015

Cool, that's fair enough. My thinking behind that suggestion was that looking forward to apps it would be good to ensure that any core UI components are re-usable.

There's a push to get zelda shipped so let's focus on functionality for now :)

@acburdine

This comment has been minimized.

Member

acburdine commented Aug 1, 2015

Ah okay I see your thinking now 馃槃 tag component still needs drag and drop too, so down the road it will need some reworking anyways

@acburdine acburdine force-pushed the acburdine:tag-component branch 2 times, most recently from 4e274d2 to 776c3d6 Aug 2, 2015

@ErisDS

This comment has been minimized.

Member

ErisDS commented Aug 3, 2015

Hey @acburdine have taken this for a quick spin, at first, I couldn't seem to get an new tag to save. I think though it's something to do with the fact that clicking off of the tag component doesn't complete the last tag and there is no save triggered?

@ErisDS

This comment has been minimized.

Member

ErisDS commented Aug 3, 2015

I'm seeing a lot of bugs with this:

  • Create a new post with a brand new tag, and press save. Refresh and the tag is gone.
  • Defocus from the component, the tag is not completed, there is no save
  • When the tag selection drop down is open, clicking on an option doesn't choose that option
  • I think I've also seen a ghost tag on a new post, but not reproduced that again
@acburdine

This comment has been minimized.

Member

acburdine commented Aug 3, 2015

@ErisDS I'll try and get all of those fixed today.

@ErisDS

This comment has been minimized.

Member

ErisDS commented Aug 4, 2015

Reproduction steps for the 2 gnarly bugs:

Create a new post with a brand new tag, and press save. Refresh and the tag is gone.

Steps:

  • From the content screen, hit 'New Post'
  • Notice that the cursor is in the title, type a title, and save
  • Hit 'New Post' again from the editor
  • Notice that the cursor is nowhere to be seen, without putting the cursor anywhere, type something (as though you had expected that the title be focused)
  • Open the post setting menu and you should see your typing went into the tag component

ghost tag on a new post

  • Open an existing post in the editor
  • Open the PSM and type a tag, do not press enter or complete the tag in any way
  • Hit the 'New Post' button
  • Open the PSM and your unfinished tag is there

With the first one, the title should always have focus when opening a new post, when editing an existing post the focus is not meant to be on any input.

With the second, making it so that defocusing the component results in the last tag being completed and saved, should help?

@acburdine acburdine force-pushed the acburdine:tag-component branch from 776c3d6 to 6319cd3 Aug 4, 2015

@acburdine

This comment has been minimized.

Member

acburdine commented Aug 4, 2015

@ErisDS I fixed all the bugs. I was getting a weird Casper error on local but maybe it was just me, will see what the Travis tests rustle up.

@acburdine acburdine changed the title from [WIP] Re-implement tag-editing component to Re-implement tag-editing component Aug 4, 2015

@acburdine acburdine force-pushed the acburdine:tag-component branch from 6319cd3 to d7a29d8 Aug 4, 2015

bindKeys: function () {
var self = this;
key('enter, tab, ,', 'tags', function (event) {

This comment has been minimized.

@ErisDS

ErisDS Aug 6, 2015

Member

Does this take into account that the comma is different in different locales? The original code was:

isComma = ','.localeCompare(String.fromCharCode(event.keyCode || event.charCode)) === 0;

This has regressed twice before, so I'm keen to avoid it happening again.

This comment has been minimized.

@acburdine

acburdine Aug 7, 2015

Member

I think it's fixable, I'll have to play around with the keymaster library to see though.

This comment has been minimized.

@ErisDS

ErisDS Aug 8, 2015

Member

Shall we have a separate issue to finish that off?

@ErisDS

This comment has been minimized.

Member

ErisDS commented Aug 6, 2015

This is now working much better 馃憤 - I think this works as expected - I have just one outstanding question about the comma behaviour.

@jaswilli @kevinansfield any code review comments?

@acburdine acburdine force-pushed the acburdine:tag-component branch from d7a29d8 to 6b54ce0 Aug 10, 2015

@acburdine

This comment has been minimized.

Member

acburdine commented Aug 10, 2015

@ErisDS I added a check for the isComma behavior, and it seemed to work on my end, so all should be good now 馃槃

@ErisDS

This comment has been minimized.

Member

ErisDS commented Aug 10, 2015

Eek there's a linting error 馃槥 other than that looks good to go 馃憤

@acburdine acburdine force-pushed the acburdine:tag-component branch from 6b54ce0 to e537ab1 Aug 10, 2015

reimplement tag editing component for posts
refs #3800
- remove old tag editor code
- reimplement tag editor as an ember component
- add tag editor component to PSM

@acburdine acburdine force-pushed the acburdine:tag-component branch from e537ab1 to 2c5d2d6 Aug 10, 2015

ErisDS added a commit that referenced this pull request Aug 10, 2015

Merge pull request #5553 from acburdine/tag-component
Re-implement tag-editing component

@ErisDS ErisDS merged commit 7090b4c into TryGhost:master Aug 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@acburdine acburdine deleted the acburdine:tag-component branch Aug 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment