-
Notifications
You must be signed in to change notification settings - Fork 65
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: add template.visibleFiles
option
#165
base: main
Are you sure you want to change the base?
Changes from all commits
a62e777
57adec8
f498eb2
8f59cf9
5d050dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,10 +42,18 @@ export class TutorialStore { | |
private _ref: number = 1; | ||
private _themeRef = atom(1); | ||
|
||
/** Files from lesson's `_files` directory */ | ||
private _lessonFiles: Files | undefined; | ||
|
||
/** Files from lesson's `_solution` directory */ | ||
private _lessonSolution: Files | undefined; | ||
|
||
/** All files from `template` directory */ | ||
private _lessonTemplate: Files | undefined; | ||
|
||
/** Files from `template` directory that match `template.visibleFiles` patterns */ | ||
private _visibleTemplateFiles: Files | undefined; | ||
|
||
/** | ||
* Whether or not the current lesson is fully loaded in WebContainer | ||
* and in every stores. | ||
|
@@ -165,15 +173,17 @@ export class TutorialStore { | |
|
||
signal.throwIfAborted(); | ||
|
||
this._lessonTemplate = template; | ||
this._lessonFiles = files; | ||
this._lessonSolution = solution; | ||
this._lessonTemplate = template; | ||
this._visibleTemplateFiles = pick(template, lesson.files[1]); | ||
|
||
this._editorStore.setDocuments(files); | ||
const editorFiles = { ...this._visibleTemplateFiles, ...this._lessonFiles }; | ||
this._editorStore.setDocuments(editorFiles); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed that the files from the template are not read-only. Given that they aren't part of the lesson but are here to provide information on the project, I really feel like they should be read only. Otherwise it feels like we're moving too far away from the original goal of having tutorials be this "safe" environment where learners can focus on the learning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we have options to make any file readonly at the moment. Maybe we need such feature, especially once adding new files via terminal becomes possible? Currently it's intentional that files are modifiable. This is simply to reduce repetition - you don't have to copy-paste files in every single lesson just to make them visible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is correct, until this PR we didn't have a need for them. I'm not quite sure what we want to do about files being added from a terminal or more generally added in WebContainer by any mean, but I'm tempted to think that we'll want to design a solution that around a specific example.
Then IMO it's the wrong abstraction. I never saw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, it sounds like we need to reconsider these changes. Maybe it's best to have only files from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this option shouldn't even be on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that's a good point actually! It's not really a template property but more like what we should show from the fs regardless of where it's coming from. Hmm I agree maybe this is something we should discuss further, maybe we could start a thread on twitter about it and ping people that have requested the feature? Or maybe we can have the conversation in a GitHub issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like:
|
||
|
||
if (lesson.data.focus === undefined) { | ||
this._editorStore.setSelectedFile(undefined); | ||
} else if (files[lesson.data.focus] !== undefined) { | ||
} else if (editorFiles[lesson.data.focus] !== undefined) { | ||
this._editorStore.setSelectedFile(lesson.data.focus); | ||
} | ||
|
||
|
@@ -283,8 +293,10 @@ export class TutorialStore { | |
return; | ||
} | ||
|
||
this._editorStore.setDocuments(this._lessonFiles); | ||
this._runner.updateFiles(this._lessonFiles); | ||
const files = { ...this._visibleTemplateFiles, ...this._lessonFiles }; | ||
|
||
this._editorStore.setDocuments(files); | ||
this._runner.updateFiles(files); | ||
} | ||
|
||
solve() { | ||
|
@@ -294,7 +306,7 @@ export class TutorialStore { | |
return; | ||
} | ||
|
||
const files = { ...this._lessonFiles, ...this._lessonSolution }; | ||
const files = { ...this._visibleTemplateFiles, ...this._lessonFiles, ...this._lessonSolution }; | ||
|
||
this._editorStore.setDocuments(files); | ||
this._runner.updateFiles(files); | ||
|
@@ -361,3 +373,15 @@ export class TutorialStore { | |
return this._runner.takeSnapshot(); | ||
} | ||
} | ||
|
||
function pick<T>(obj: Record<string, T>, entries: string[]) { | ||
const result: Record<string, T> = {}; | ||
|
||
for (const entry of entries) { | ||
if (entry in obj) { | ||
result[entry] = obj[entry]; | ||
} | ||
} | ||
|
||
return result; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'This file is only present in template'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patterns might look a bit weird. It's matching the words on specific rows. I don't think expressive-code supports syntax like
ins={24, "first.js"}
to matchfirst.js
on line 24 only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing
_files/src
here 😱