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

Exclude file ext from initial selection when renaming a file. #7209

Merged
merged 1 commit into from Mar 18, 2014

Conversation

Projects
None yet
5 participants
@Robo210

Robo210 commented Mar 16, 2014

This change performs the rename as usual through JSTree, then
grabs the DOM node and adjusts the selection on it after the fact.
This fixes issue #7019

Kyle Sabo
Exclude file ext from initial selection when renaming a file.
This change performs the rename as usual through JSTree, then
grabs the DOM node and adjusts the selection on it after the fact.
This fixes issue #7019
@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 17, 2014

Contributor

Initial review done, works & looks fine.

Contributor

zaggino commented Mar 17, 2014

Initial review done, works & looks fine.

@ingorichter

This comment has been minimized.

Show comment
Hide comment
@ingorichter

ingorichter Mar 18, 2014

Contributor

@zaggino could you please finish this review and when you are done please merge. Thanks.

Contributor

ingorichter commented Mar 18, 2014

@zaggino could you please finish this review and when you are done please merge. Thanks.

_projectTree.jstree("rename");
var indexOfExtension = escapedName.lastIndexOf('.');

This comment has been minimized.

@peterflynn

peterflynn Mar 18, 2014

Member

Should it be first index of "." instead? If I have e.g. "foo.css.erb" I'd just want to rename the "foo" part.

@peterflynn

peterflynn Mar 18, 2014

Member

Should it be first index of "." instead? If I have e.g. "foo.css.erb" I'd just want to rename the "foo" part.

This comment has been minimized.

@zaggino

zaggino Mar 18, 2014

Contributor

When renaming files like less-1.4.2.min.js, usually only the last part is considered an extension.

@zaggino

zaggino Mar 18, 2014

Contributor

When renaming files like less-1.4.2.min.js, usually only the last part is considered an extension.

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

Ignore comment, good to merge. My blind self.

Contributor

zaggino commented Mar 18, 2014

Ignore comment, good to merge. My blind self.

@zaggino zaggino removed the needs review label Mar 18, 2014

zaggino added a commit that referenced this pull request Mar 18, 2014

Merge pull request #7209 from Robo210/initial_selection
Exclude file ext from initial selection when renaming a file.

@zaggino zaggino merged commit 8670907 into adobe:master Mar 18, 2014

1 check passed

default The Travis CI build passed
Details
@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

@peterflynn - this could be done as a separate PR because it's a bit more complicated. Should we search the languages.json for known extensions that have . in them and use this for selecting?

Contributor

zaggino commented Mar 18, 2014

@peterflynn - this could be done as a separate PR because it's a bit more complicated. Should we search the languages.json for known extensions that have . in them and use this for selecting?

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 18, 2014

Member

Seems plausible, but I don't know if we have to get that fancy. Your argument above sounded reasonable enough to me -- let's wait & see if we get any feedback about it?

Member

peterflynn commented Mar 18, 2014

Seems plausible, but I don't know if we have to get that fancy. Your argument above sounded reasonable enough to me -- let's wait & see if we get any feedback about it?

@Denisov21

This comment has been minimized.

Show comment
Hide comment
@Denisov21

Denisov21 Mar 18, 2014

Contributor

Hi,
I think it works well now. I share the opinion on "foo.css.erb" is only the extension *.erb. Thanks for the work guys!

Contributor

Denisov21 commented Mar 18, 2014

Hi,
I think it works well now. I share the opinion on "foo.css.erb" is only the extension *.erb. Thanks for the work guys!

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

I don't know - I realized I like the fancy idea and also implementation was just a few lines so I made up a PR and I'll leave to others to merge or not to merge :)

Contributor

zaggino commented Mar 18, 2014

I don't know - I realized I like the fancy idea and also implementation was just a few lines so I made up a PR and I'll leave to others to merge or not to merge :)

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

Btw @peterflynn we do not longer use doublequotes " for strings?
I don't see any "quotmark": "double" in https://github.com/adobe/brackets/blob/master/.jshintrc but I remember I was told to do that before.

Contributor

zaggino commented Mar 18, 2014

Btw @peterflynn we do not longer use doublequotes " for strings?
I don't see any "quotmark": "double" in https://github.com/adobe/brackets/blob/master/.jshintrc but I remember I was told to do that before.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 18, 2014

Member

@zaggino Ah, yes -- all JS code should use double-quoted strings (per style guide). So the string in the lastIndexOf() call here should be fixed. Could you roll that into your PR?

Member

peterflynn commented Mar 18, 2014

@zaggino Ah, yes -- all JS code should use double-quoted strings (per style guide). So the string in the lastIndexOf() call here should be fixed. Could you roll that into your PR?

@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Mar 18, 2014

Contributor

Already did that for PR. Also tried using "quotmark": "double" in JSHint and it blew out a lof of ' usage, especially in test/ but also 20 or so in src/

Contributor

zaggino commented Mar 18, 2014

Already did that for PR. Also tried using "quotmark": "double" in JSHint and it blew out a lof of ' usage, especially in test/ but also 20 or so in src/

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Mar 19, 2014

Member

Seems worth fixing the ones in src at least, though not high priority...

Member

peterflynn commented Mar 19, 2014

Seems worth fixing the ones in src at least, though not high priority...

@Robo210 Robo210 deleted the Robo210:initial_selection branch Mar 19, 2014

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