Skip to content

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

Merged
merged 8 commits into from
Jun 20, 2024

Conversation

Nemikolh
Copy link
Member

This PR makes it possible to specify the ordering in the metadata of the tutorial, part or chapter. If it isn't specified it fallback to the current behaviour and when it's used it prints warning if it finds any issues:

image

How to specify the order

In tutorial/meta.md:

type: tutorial
parts:
  - foo         # Must match the name of the folder containing the part
  - bar

In tutorial/foo/meta.md:

type: part
chapters:
  - chapterfoo  # Must match the name of the folder containing the chapter

In tutorial/foo/chapterfoo/meta.md:

type: chapter
lessons:
  - welcome    # Must match the name of the folder containing the lesson

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Jun 19, 2024

⚠️ No Changeset found

Latest commit: 9170756

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Nemikolh Nemikolh requested review from SamVerschueren and d3lm and removed request for SamVerschueren June 19, 2024 14:30
Copy link
Contributor

@SamVerschueren SamVerschueren left a 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).

Comment on lines 307 to 308
if (splitA[i] !== splitB[i]) {
return splitA[i].localeCompare(splitB[i]);
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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:

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.

Comment on lines -8 to -9
const chapter = part.chapters[1];
const lesson = chapter.lessons[1];
Copy link
Contributor

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?

Copy link
Member Author

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! 😃

Comment on lines +7 to +8
const chapter = part.chapters[part?.firstChapterId!];
const lesson = chapter.lessons[chapter?.firstLessonId!];
Copy link
Contributor

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?

Copy link
Member Author

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.

@Nemikolh Nemikolh requested a review from SamVerschueren June 20, 2024 13:47
Copy link
Contributor

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

Copy link

cloudflare-workers-and-pages bot commented Jun 20, 2024

Deploying tutorialkit-demo-page with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9170756
Status:🚫  Build failed.

View logs

Copy link

Deploying tutorialkit-docs-page with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9170756
Status:🚫  Build failed.

View logs

@Nemikolh Nemikolh merged commit 2b131e5 into main Jun 20, 2024
7 of 9 checks passed
@Nemikolh Nemikolh deleted the joan/order-from-config branch June 20, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants