Comment publish preview flow

[begedin]

Implemented

• we have an editor-with-preview component, which has two "tabs". It starts in editing mode by default and shows a regular text area.
• when user clicks preview mode, several things happen
  • the "preview" field is set to "Loading preview..."
  • the mode is set to "previewing", causing the preview tab to display and the "loading preview..." text to render
  • an action called generatePreview is triggered, with the textarea content as a parameter.
• integrated the control into all 4 scenarios - new post, new comment, edited post, edited comment
• added separate editing of existing post title and body
  • for title, we have a post-title component. which handles show and edit mode as necessary

[joshsmith]

This is good. I like the fact that this is so data-down, actions-up, and can be reused. Good thinking here. 👍

[joshsmith]

This will also make it easier to use it in-line when editing posts/comments that are already published.

[begedin]

The flow is done, but this still needs work. There's a lot of things to cover and, while I wrote spec first, I can't be sure I covered it all, so I'm slowly going through the app and figuring it out manually, updating and adding new specs as I go along.

I could use a second pair of eyes on this.

I'll also go through the code and rewrite anything that ended up being ugly, now that the major specs are covering the behavior. I'll also add comments to anything that's unclear.

[begedin]

@joshsmith I think this is in a mergeable state.

I wrote what I believe are thorough specs, tested manually to ensure everything is consistent with the API and all of the features are covered.

The styles are added are not intended to be final. It's just something I put in so I can actually see what I'm clicking at in the UI. We should probably work on styling in a separate PR.

I also took the liberty of getting rid of all the deprecation errors I could detect. They consisted of 3 groups in total

• ember-moment was giving out warnings about blank values being passed into it. I got rid of that by adding a config flag.
• record relationships will be async by default in the future. They are not async by default right now, so there was a deprecation warning for that. I explicitly set all record relationships to async, since I believe we agreed this is what we agreed on.
• the adapter will reload records in the background by default in the future. right now, it does not. I decided t define a shouldBackgroundReloadRecord method on our application adapter, which returns false (our old behavior), since we don't want anything unexpected to happen in the future. We can revisit that when the default behavior actually changes and we decide we can find a use for background reloading.

[joshsmith]

Should we have an open issue for background loading records to remind ourselves when it does change?

[begedin]

Good idea, I'll make one.

[joshsmith]

And thanks for getting rid of those warnings. The notices for async relationships in particular were a pain.

[joshsmith]

Some things I'm noticing while working through this:

Post creation without a title gives you something like:

The adapter rejected the commit because it was invalid

Ideally we should try to give the user a better error.

Switching to the editing tab on posts/comments causes a 422 error and "Loading preview..." to render. We should maybe not send the body at all in this case and just render a blank preview.

[joshsmith]

We may want to refactor this later with the currentUser object I'm introducing in #76.

[joshsmith]

@begedin and maybe also @hbrysiewicz can answer, I notice that didInitAttrs is being used a lot here. Is there any reason for doing this instead of setting the attribute in the component object itself?

[hbrysiewicz]

Common to keep the aliases all together near the top after static attributes, then computed properties, then methods, then actions. Keeps the files clean and clear.

[joshsmith]

Sorry, I'm having trouble seeing how you want this reordered. Can you order it in something like:

• classNames
• classNameBindings
• canEdit
• whateverElse

so I can understand better?

[hbrysiewicz]

I was actually wondering the same thing, @joshsmith but I know that didInitAttrs is newer to Ember and I've been out of the game a bit. I remember hearing a lot about it on Taras' Global Ember Meetup episode on CRUD in Ember 2... but can't remember context/purpose.

I am having trouble using the UI (edit buttons overlapping other elements) but I think it was mentioned this should be a separate issue, correct? Functionally it works.

Otherwise my only feedback was on the organization of file for clarity. Looks wonderful! Really like how clear the functionality is with actions and naming. Awesome PR

[joshsmith]

By having trouble using UI, you just mean the design is poor, right? That's expected, if so. If it's a "something didn't work", then that's not expected.

[hbrysiewicz]

Yes @joshsmith just that its poor design :) functionally working so +1

[joshsmith]

I've added #83, #84, #85, and #86 as follow-up tasks for this. Also there are design follow-up tasks that will come when @jimmynotjim helps figure out UI of this, and we can implement.