-
Notifications
You must be signed in to change notification settings - Fork 85
feat: allow ordering to be config based in addition to folder based #79
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
Conversation
|
|
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's a bit hard to follow for me, but maybe it is because I don't really know how it worked before (except that it was based on the folder name).
if (splitA[i] !== splitB[i]) { | ||
return splitA[i].localeCompare(splitB[i]); |
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.
I thought the idea was to also keep the current behaviour if no order was manually specified. But this means that 10-foo
comes before 9-bar
?
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.
Actually I'll remove that entire for loop. It's not needed anymore and it's confusing.
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.
So the current behaviour still works then?
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! It's not handled here, it's handled there:
tutorialkit/packages/astro/src/default/utils/content.ts
Lines 286 to 292 in ea6d42b
function getOrder(order: string[] | undefined, fallbackSourceForOrder: Record<string, any>): string[] { | |
if (order) { | |
return order; | |
} | |
return Object.keys(fallbackSourceForOrder).sort((a, b) => Number(a) - Number(b)); | |
} |
The last line is what is supposed to make this ordered by default with the numbering.
However I got it wrong! It relies on the fact that Number('123-foobar') === 123
' which is wrong. Instead it's parseInt('123-foobar', 10);
that we were using before. I'll fix that now.
const chapter = part.chapters[1]; | ||
const lesson = chapter.lessons[1]; |
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 was this 1-based?
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.
The folder were expected to start at 1
. We now instead pick the first folder that comes up. It could start at anything: 0, 4, ... or be explicitly defined.
So this is a new tiny feature! 😃
const chapter = part.chapters[part?.firstChapterId!]; | ||
const lesson = chapter.lessons[chapter?.firstLessonId!]; |
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.
I'm very confused. So all the firstPartId
and firstChapterId
and firstLessonId
are defined as optional in entities/index.ts
. Why are we here telling TS that they will be defined?
Also, what if they are defined but they don't exist? Then chapter
will be undefined and it will throw, no?
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.
Correct! It's actually the same behaviour as before because there would be no part either.
Note to be in that situation you must not have a part, nor a lesson nor a chapter. Because if you have any, the getTutorial
will throw first.
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.
Great work!
This PR makes it possible to specify the ordering in the metadata of the
tutorial
,part
orchapter
. If it isn't specified it fallback to the current behaviour and when it's used it prints warning if it finds any issues:How to specify the order
In
tutorial/meta.md
:In
tutorial/foo/meta.md
:In
tutorial/foo/chapterfoo/meta.md
: