Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Join 2 articles public form views into one for sake of DRY #339

Closed
wants to merge 1 commit into from

Conversation

ramusus
Copy link

@ramusus ramusus commented Jan 1, 2015

Solves #336

@andresmanz
Copy link

In my opinion it is extremely helpful to have best practices in demo code. Is there a reason this hasn't been merged yet?

@codydaig
Copy link
Member

codydaig commented Sep 7, 2015

@ramusus Would you mind updating this PR against the latest master branch? I'd really like to see this change myself. Unless there's any objections of moving to a dry structure? @lirantal @ilanbiala @mleanos @trainerbill @rhutchison

$scope.article.title = '';
$scope.article.content = '';
}
var error = function(errorResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this doing now?

@ilanbiala
Copy link
Member

@ramusus this looks fine to me, albeit some changes that need to be made such as removing data- and fixing the indentation.

@ilanbiala ilanbiala self-assigned this Sep 7, 2015
@ilanbiala ilanbiala added this to the 0.4.2 milestone Sep 7, 2015
@ilanbiala
Copy link
Member

@ramusus would you be able to tidy up this PR so it can be merged?

@ramusus
Copy link
Author

ramusus commented Sep 14, 2015

Sorry, unfortunatly I'm very busy now

@mleanos
Copy link
Member

mleanos commented Sep 14, 2015

@ilanbiala @codydaig

This looks like a good implementation. Since @ramusus doesn't have time for this, I'll take it over. This is related to #807 & #874 Once the latter is decided on, then I will work on this implementation.

@ilanbiala
Copy link
Member

@mleanos why do you need #874 to work on this?

@mleanos
Copy link
Member

mleanos commented Oct 12, 2015

@ilanbiala #874 is a refactor of the Article module. However, since that PR is set for 0.5.0 then this can probably move forward without consideration for both #807 and #874

@codydaig
Copy link
Member

@ilanbiala Can this be moved to 0.5.0?

@ilanbiala ilanbiala modified the milestones: 0.5.0, 0.4.2 Oct 14, 2015
@ilanbiala
Copy link
Member

mleanos do you need #874 to work on this?

@mleanos
Copy link
Member

mleanos commented Dec 7, 2015

@ilanbiala I'm not sure. I need to take another look at this implementation. I'd rather wait until the dust settles with #874 since it's a huge refactor, and this implementation might not be relevant once we implement John Papa's styleguide; I'm not exactly sure though.

Also, #807 will introduce a change in how we're allowing users to edit/create Articles, it may make sense to hold off on this; or just address these concerns in #807

@rhutchison
Copy link
Contributor

@ramusus I agree with the direction, but not completely with the implementation. The controller should call $save and the service should work out whether it's an insert or update.

@codydaig @mleanos

@mleanos
Copy link
Member

mleanos commented Dec 29, 2015

@rhutchison I agree. I would like to reintroduce this with #807. Also, would prefer to add the angular styleguide with it, since the article refactor PR has stalled for so long. We need to get moving on the styleguide, so we can have it out with the 0.5.0 release.

rhutchison added a commit to Gym/mean that referenced this pull request Jan 1, 2016
Update the articles module to implement the style guidelines.

Much of this work is from @trainerbill

Closes meanjs#874
Closes meanjs#339
rhutchison added a commit to Gym/mean that referenced this pull request Jan 2, 2016
Update the articles module to implement the style guidelines.

Much of this work is from @trainerbill

Closes meanjs#874
Closes meanjs#339
@ilanbiala ilanbiala assigned codydaig and unassigned ilanbiala Jan 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants