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

feat(relatedFiles): Add more open related files support #102

Merged

Conversation

@hiaux0
Copy link
Contributor

hiaux0 commented Aug 12, 2019

Feature

Additionally to the existing 'open related files', one is now able to directly switch to other related files.
Namely,

  • scripts: eg. .js, .ts
  • styles: eg. .css, .less
  • unit: eg. .spec.js, .spec.ts
  • view: eg. .html

All the extensions are configurable view the following setting

	"aurelia.relatedFiles": {
		"script": ".ts",
		"style": ".less",
		"unit": ".spec.ts",
		"view": ".html"
	},

au-ext-related-files

Motivation

I want to quickly switch to all related files to an Aurelia component.

What was done

  • add settings
  • add 4 additional commands for related files to open files directly, instead of just toggling between .js and .html
  • Changed logic from existing openRelated to get script extension from settings
    (motivation: I believe an aurelia project will mostly have either only .js or .ts files)
return workspace.getConfiguration().get<AureliaConfigProperties['relatedFiles']>('aurelia.relatedFiles', defaultSettings);
}

private async onOpenRelated(editor: TextEditor, edit: TextEditorEdit) {

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 12, 2019

Author Contributor

Changed logic from existing openRelated to get script extension from settings
(motivation: I believe an aurelia project will mostly have either only .js or .ts files)

This comment has been minimized.

Copy link
@eriklieben

eriklieben Aug 15, 2019

Member

Not sure about this, but it might be more logical to remove the command 'open related aurelia file'. It feels a bit inconsistent together with the newly created commands that all specify what they will open.

You need to remember now that is is basically an alias for open view or open script. However, it can be handy if you use it a lot because it will be on top of the command panel and then you don't need to switch between view and script in the list or it could be one keyboard combination if you bind it to that.
With only the new commands available this would always require at least 2 key combinations, but then again you could also just create them for all 4 options, remember them and switch like a ninja between those 4, which might be more helpfull in the end.

In the following case the 'open related aurelia file' is a bit odd I think (was also the case in the current version, but less 'visible'):

Let's say you have a view-only component like

<template bindable="foo,bar">
  <require from="boo.css" />
  <h1>${foo}</h1>
  <p>${bar}</p>
</template>

If I would now use the command 'open related aurelia file' would I then expect it to open the related CSS file, because the CSS file is the only related file to the HTML-only component :-) Instead I would need to use the open styles command.

How do you feel about removing the option 'open related aurelia file'?


Currently out of the box the command will only work in projects that use .js, but I think we can fix that with the modification of the default settings to arrays.

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 16, 2019

Author Contributor

I also thought about removing it, but then again, this ninja switching is indeed handy.

How about renaming eg.

- open related aurelia file
+ switch between aurelia view and viewModel

As for the other possible combinations, I'd say we get the first version in

  • 4 target switching
  • (maybe renamed 'open related')
    and get see the other as an enhancement for later?

This comment has been minimized.

Copy link
@eriklieben

eriklieben Aug 16, 2019

Member

switch between aurelia view and viewModel

sounds great. super idea; let's do that :-)

@hiaux0 hiaux0 changed the title Feat/related files more files support v3 Add more open related files support [Feat/relatedFiles] Aug 12, 2019
@EisenbergEffect EisenbergEffect requested a review from eriklieben Aug 12, 2019
@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Aug 12, 2019

@eriklieben Looks like @hiaux0 has some neat ideas to improve the plugin 😄

@hiaux0

This comment has been minimized.

Copy link
Contributor Author

hiaux0 commented Aug 12, 2019

Re failing build: I don't know how to fix this :(

image

@hiaux0 hiaux0 changed the title Add more open related files support [Feat/relatedFiles] feat(relatedFiles): Add more open related files support Aug 13, 2019
@eriklieben eriklieben self-assigned this Aug 13, 2019
@eriklieben

This comment has been minimized.

Copy link
Member

eriklieben commented Aug 13, 2019

Awesome! I will look into the typescript definition error tonight or tomorrow morning.

@eriklieben

This comment has been minimized.

Copy link
Member

eriklieben commented Aug 15, 2019

build should be fixed when you merge master into it

package.json Outdated Show resolved Hide resolved
@hiaux0 hiaux0 force-pushed the hiaux0:feat/related-files-more-files-support-v3 branch from 79b6a44 to 47817b9 Aug 16, 2019
Copy link
Contributor Author

hiaux0 left a comment

Thx @eriklieben for suggesting the better out-of-the-box experience!

Now a bit more complicated to work with, since we need to operate on arrays now, , but no big deal.
(I have the later go-to PR in mind)

Some replace workaround needed unfortunately

};
return workspace.getConfiguration().get<AureliaConfigProperties['relatedFiles']>('aurelia.relatedFiles', defaultSettings);

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 16, 2019

Author Contributor

Enough to use this default config object

let fileName = `${path.basename(fullPath, path.extname(fullPath))}${ext}`
.replace('.spec.spec', '.spec'); // Quick fix because we are appending eg. '.spec.ts' to 'file.spec'
fullPath = path.join(path.dirname(fullPath), fileName);
if (!fs.existsSync(fullPath)) return;

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 16, 2019

Author Contributor

We are now able to check synchronously if file exists.

* '.spec' is not recognized as an file extension.
* Thus, `replace`, so we are able to switch from, eg. 'unit' to 'style'.
* */
const fileName = editor.document.fileName.replace('.spec', '');

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 16, 2019

Author Contributor

replace ugliness :(

let targetFile: string;
relatedExts.forEach(ext => {
let fileName = `${path.basename(fullPath, path.extname(fullPath))}${ext}`
.replace('.spec.spec', '.spec'); // Quick fix because we are appending eg. '.spec.ts' to 'file.spec'

This comment has been minimized.

Copy link
@hiaux0

hiaux0 Aug 16, 2019

Author Contributor

replace ugliness :(

@eriklieben eriklieben merged commit 1caecb9 into aurelia:master Aug 18, 2019
5 checks passed
5 checks passed
WIP Ready for review
Details
build_and_test Workflow: build_and_test
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details
@hiaux0 hiaux0 deleted the hiaux0:feat/related-files-more-files-support-v3 branch Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.