Skip to content

Conversation

@VarunAgw
Copy link
Contributor

Fixes #188

@iandunn
Copy link
Contributor

iandunn commented Aug 12, 2014

This looks good, except for 097a0aa:

  • It should be a separate PR, since it's not related to the ctrl-enter issue.
  • It fixes Conversation textarea should auto-expand #59, not Set From header #159
  • I'm leery of including an external library; how do we know it's secure?
  • If we do include it, jquery.autosave.min.js should include in the min.js file
  • The min.js file needs to be regenerated, either as part of the commit that changed any of the js files, or as a separate commit. If it's separate commit, it needs to be done after any other commits that touch JS files.

@VarunAgw
Copy link
Contributor Author

It should be a separate PR, since it's not related to the ctrl-enter issue.

Yeah! I was lazy. I thought if I create a new PR it will cause merge conflict issue because of $-jquery conversion commit

It fixes #59, not #159

Sorry a typo. Does it need to changed or we can leave it as it is this time

I'm leery of including an external library; how do we know it's secure?

I thought since it is actively developed from last three years with more than a dozen contributors, I though we can trust it.

If we do include it, jquery.autosave.min.js should include in the min.js file

You mean supportflow.min.js? I thought it is external resource with different version system, it will be good to keep it as a different file. Just my opinions..What do you think?

The min.js file needs to be regenerated, either as part of the commit that changed any of the js files, or as a separate commit. If it's separate commit, it needs to be done after any other commits that touch JS files.

Okay.

@iandunn
Copy link
Contributor

iandunn commented Aug 12, 2014

I thought if I create a new PR it will cause merge conflict issue because of $-jquery conversion commit

That's true, but mixing PRs can cause problems, too. For example, the ctrl-enter commits are fine, but the auto-resize commit has several issues, so the entire PR is delayed, which can lead to merge conflicts with other PRs. It also makes it harder to find the auto-resize commit in the future, since it's buried in an unrelated PR.

I think you should revert the auto-resize commit and make it a new PR, so that we can go ahead and merge the rest of the PR. When you do, add a note to the new one to reference the discussion on this PR, since it will be relevant to the new one.

I thought since it is actively developed from last three years with more than a dozen contributors, I though we can trust it.

Not necessarily; the trust is based more on the experience level of the developer(s) and quality of the code, rather than the popularity. Popularity can help a project be more secure because it means more people are inspecting the code, but it's very common for popular projects to have security vulnerabilities, even very severe and relatively obvious ones.

It's a pretty small script, though, so read through it to see if you notice any problems. If not, we can go ahead and use it.

You mean supportflow.min.js? I thought it is external resource with different version system, it will be good to keep it as a different file. Just my opinions..What do you think?

It's correct to use 1.18.9 as the version number when SCRIPT_DEBUG is on, but when it's off everything should be merged together, regardless of whether it's an external library of part of SupportFlow core.

This feature is now transferred to its own new branch
This reverts commit 097a0aa.
@VarunAgw
Copy link
Contributor Author

It's correct to use 1.18.9 as the version number when SCRIPT_DEBUG is on, but when it's off everything should be merged together, regardless of whether it's an external library of part of SupportFlow core.

Using 1.18.9 as version in dev while 0.3 in production is not possible because of how SupportFlow works. So, I think we should keep it 0.3 in both case.
https://github.com/SupportFlow/supportflow/blob/master/supportflow.php#L872

@VarunAgw
Copy link
Contributor Author

It's a pretty small script, though, so read through it to see if you notice any problems. If not, we can go ahead and use it.

It seems okay to me. But I have not developed any jquery plugin so I might miss something. https://github.com/jackmoore/autosize/blob/master/jquery.autosize.js

Also I have created a branch for reply-textarea locally so this can be merged now if there are no other issues

@iandunn
Copy link
Contributor

iandunn commented Aug 12, 2014

Using 1.18.9 as version in dev while 0.3 in production is not possible

That's fine.

It seems okay to me. But I have not developed any jquery plugin so I might miss something.

I read through it too and didn't notice any problems, so let's go ahead with it.

iandunn added a commit that referenced this pull request Aug 12, 2014
Enable ctrl-enter submission on Unix-based operating systems.
@iandunn iandunn merged commit e586045 into SupportFlow:master Aug 12, 2014
@VarunAgw VarunAgw deleted the ctrl-enter branch August 18, 2014 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cntl+Enter doesn't submit reply

2 participants