Skip to content
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

♿️ Restore platform-centric menu names for showing in file manager #1311

Merged
merged 2 commits into from Apr 18, 2019

Conversation

Projects
None yet
4 participants
@mattlubner
Copy link
Contributor

commented Apr 12, 2019

Description of the Change

Addresses #1307. Restore platform-centric menu names for showing files in the user's file manager.

Benefits

  • Following each platform's idiomatic naming conventions will improve usability.

Possible Drawbacks

  • The name of the platform's default file manager is always used. This will be slightly misleading if shell.showItemInFolder opens a third-party file manager.

Applicable Issues

@mattlubner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Hey @rsese @50Wliu, how do we proceed? Should I attempt to build Atom locally, write a test, or do something else?

This is the first time I've written any Coffeescript (or contributed to Atom), so any guidance / input is appreciated! 🙏

@@ -560,13 +560,22 @@ class TreeView
return unless filePath = @selectedEntry()?.getPath()

unless shell.showItemInFolder(filePath)
atom.notifications.addWarning("Unable to show #{filePath} in file manager")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")

This comment has been minimized.

Copy link
@lee-dohm

lee-dohm Apr 17, 2019

Member

We should probably leave the case unmodified here.

Suggested change
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName()}")

showCurrentFileInFileManager: ->
return unless filePath = atom.workspace.getCenter().getActiveTextEditor()?.getPath()

unless shell.showItemInFolder(filePath)
atom.notifications.addWarning("Unable to show #{filePath} in file manager")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")

This comment has been minimized.

Copy link
@lee-dohm

lee-dohm Apr 17, 2019

Member

We should probably leave the case unmodified here.

Suggested change
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName().toLowerCase()}")
atom.notifications.addWarning("Unable to show #{filePath} in #{@getFileManagerName()}")
@rsese

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Thanks @mattlubner!

Should I attempt to build Atom locally, write a test, or do something else?

No need to build Atom locally, you can follow this guide to link your modified version of tree view:

https://flight-manual.atom.io/hacking-atom/sections/contributing-to-official-atom-packages/

I confirmed that tests aren't necessary in this case but it would be good if you can manual verify that things look ok on Windows, macOS, and Linux with your changes.

@mattlubner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Thanks for the review @lee-dohm and the feedback @rsese! Just manually verified that things look good on macOS. I don't have a Window or Linux environment that I can test on, though.

@50Wliu 50Wliu merged commit 9b2d459 into atom:master Apr 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.