Skip to content

Added Git-FTP init support.#308

Merged
zaggino merged 3 commits intobrackets-userland:masterfrom
FezVrasta:gitFtp
Mar 28, 2014
Merged

Added Git-FTP init support.#308
zaggino merged 3 commits intobrackets-userland:masterfrom
FezVrasta:gitFtp

Conversation

@FezVrasta
Copy link
Copy Markdown
Contributor

  • Renamed Git-FTP "remotes" to the proper name "scopes".
  • Added Git-FTP init feature

Ready for review.

@zaggino
Copy link
Copy Markdown
Member

zaggino commented Mar 28, 2014

Could you try creating a new less stylesheet in src/Ftp/styles
and include it on demand here? Just like the .js is included.
https://github.com/zaggino/brackets-git/blob/master/main.js#L36-L38
It'd be nice to have the ftp stuff moved there.

On a second thought, don't do that now. We can't use imports yet (Solved in Sprint 38)

Comment thread src/Ftp/Ftp.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These handlers should be called the same way (chose one). Currently gitftp-remote-new is different from gitftp-remove-remote and gitftp-init-remote

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why? I don't need $(this) in handleGitFtpScopeCreation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can just rename the other two to
.on("click", ".gitftp-remove-remote", handleGitFtpScopeRemove)
and on the first line do
var $this = $(this);
OR have

.on("click", ".gitftp-remote-new", function () { handleGitFtpScopeCreation(); })

It looks more consistent this way. That's all.
Having .on("click", ".gitftp-remote-new", handleGitFtpScopeCreation) makes me think you want to pass the event from jQuery into arguments

@zaggino
Copy link
Copy Markdown
Member

zaggino commented Mar 28, 2014

Review done. Just minor stuff, quite nice 👍

@FezVrasta
Copy link
Copy Markdown
Contributor Author

Okay I've already prepared the ftp.less file and improved quality of less code.
(Next week I'll rewrite stylesheet of brackets-git from scratch)

@zaggino
Copy link
Copy Markdown
Member

zaggino commented Mar 28, 2014

@FezVrasta I don't mind you rewriting the stylesheet but directives @import doesn't work in Sprint 37 and will work only after Sprint 38 so you may want to wait for that. See adobe/brackets#7230

@zaggino zaggino merged commit 9209b62 into brackets-userland:master Mar 28, 2014
@zaggino
Copy link
Copy Markdown
Member

zaggino commented Mar 28, 2014

Whoa - finally ;-) Successful merge all through Brackets!

@zaggino
Copy link
Copy Markdown
Member

zaggino commented Mar 28, 2014

And please @FezVrasta really update the Changelog in PR's - I'll fix the English if it's bad. Users should know when you add a new stuff.

@FezVrasta FezVrasta deleted the gitFtp branch March 29, 2014 08:15
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.

2 participants