-
Notifications
You must be signed in to change notification settings - Fork 38
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: otter training - prepare training content #2223
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit c36820a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
apps/showcase/src/components/training/training-step/training-step-pres.component.scss
Outdated
Show resolved
Hide resolved
void this.loadSteps(); | ||
} | ||
|
||
public async loadResource(url: string) { |
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.
We need to put more tsdoc on this file
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.
Updated, let me know if it is not clear enough
apps/showcase/src/services/webcontainer/webcontainer.service.ts
Outdated
Show resolved
Hide resolved
{ | ||
label: 'Training', | ||
links: [ | ||
{ url: '/sdk-training', label: 'SDK Training' } |
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.
To be discussed with the team but shouldn't we review that hierarchy to have something like
Topic
|_ What is it?
|_ How to?
|_ Training
|_ Next steps
instead of having all the trainings in the same group
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.
Let's do it in another PR?
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.
yes sure
it's just to add the topic for our next discussion
} | ||
const sanitizedPath = resource.path.replace(new RegExp('^[.]/?'), ''); | ||
const parsedPath = sanitizedPath.split('/'); | ||
parsedPath.reduce((pointer, path, index) => { |
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.
let's try to see if we ca replace that with jsonPath.apply?
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.
(future PR)
/** | ||
* Index of the training step to display | ||
*/ | ||
public currentStepIndex = 0; |
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.
Why not using signal for these variables used in the 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.
Do we really need it?
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.
It was just a question for consistency I do not have strong opinion on that :)
8dc1c40
to
2eab9e8
Compare
apps/showcase/project.json
Outdated
@@ -55,7 +55,7 @@ | |||
"{projectRoot}/dev-resources", | |||
"{projectRoot}/training-assets", | |||
"{projectRoot}/*.metadata.json", | |||
"packages/@o3r/training-sdk/dist/*-structure.json" | |||
"{workspaceRoot}/packages/@o3r/training-sdk/dist/*-structure.json" |
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.
Not sure it is a good idea to add a file from another page as input.
The best practice is to add the task, from @o3r/training-sdk
, resulting to these json files in dependOn
with the ^
prefix.
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.
Updated (this will be more clear when we open the PR for with the package @o3r/training-sdk --> I can remove this modification in the current PR and add it to the next one if you prefer)
2eab9e8
to
f77e58f
Compare
f77e58f
to
89e19b9
Compare
89e19b9
to
7dc5750
Compare
7dc5750
to
c36820a
Compare
Proposed change
Preparation for the training content
Related issues