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

story: check for duplicate URL on blur #413

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
2 participants
@talklittle
Contributor

talklittle commented Oct 5, 2017

fixes #149

@pushcx

Thank you, @talklittle, for picking up one of my biggest pet peeve issues.

The code can be extracted a little differently to avoid a dependency, so I left some comments. Thanks!

checkStoryDuplicate: function(form) {
var params = $(form).serializeArray();
$.post("/stories/check_url_dupe", params, function(data) {
var da = $.parseHTML(data);

This comment has been minimized.

@pushcx

pushcx Oct 5, 2017

Member

I appreciate you being consistent with the other functions, but I don't want identifiers like data and da that don't carry any meaning. There's also no need for .parseHTML if the js isn't going to do anything with the DOM, so just .html it right into place.

@story.check_url
# ignore other types of errors (e.g., invalid URL format)
if @story.already_posted_story.blank?

This comment has been minimized.

@pushcx

pushcx Oct 5, 2017

Member

Instead of extracting all of check_url from validate, extract just the part that sets already_posted_story and maybe adds the error about it being submitted recently. Leave the regexp that checks format and adds that error.

I don't want this piece of code to have to know about all the other possible errors on URL and be maintained in sync with the model.

@@ -0,0 +1,29 @@
<div class="form_errors_header">

This comment has been minimized.

@pushcx

pushcx Oct 5, 2017

Member

Please name this file _form_errors rather than run the words together.

@pushcx pushcx merged commit f0a2065 into lobsters:master Oct 6, 2017

1 check passed

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

This comment has been minimized.

Member

pushcx commented Oct 6, 2017

Great to have this. It might be nice to use slideDown or something to make this a little less jarring, but I'd rather merge this now than wait.

@talklittle talklittle deleted the talklittle:check-dupe-story-async branch Oct 6, 2017

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