Migrate Layout File to Typescript by ong6 · Pull Request #2164 · MarkBind/markbind

ong6 · 2023-02-13T08:36:37Z

What is the purpose of this pull request?

Part of #1913

Overview of changes:
Migrate Layouts file and Layouts Manager to typescript

Anything you'd like to highlight/discuss:
N/A

Testing instructions:
N/A

Proposed commit message: (wrap lines at 72 characters)
(rebase commit)

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

jovyntls

Some comments on the types! Also, why are all the method call params (and anything comma-separated) split into a line of their own? Seems like an unnecessary change 😅

packages/core/src/Layout/Layout.ts

.eslintignore

packages/core/src/Layout/Layout.ts

jovyntls

Thanks for the PR @ong6! Can we avoid unnecessary whitespace changes (e.g. putting all variables in a newline)?

packages/core/src/Layout/Layout.ts

jovyntls

LGTM!

packages/core/src/Layout/LayoutManager.ts

raysonkoh

LGTM

raysonkoh · 2023-03-20T16:46:07Z

@ong6 could you remove the commit description in your adapt commit?

jovyntls

@ong6 Sorry I missed this earlier - could you update the imports of the files you migrated to use ES6 imports instead of require?

jovyntls

LGTM, just a rebase error!

.eslintignore

jovyntls

@ong6 Please ensure that you've fixed the merge conflicts

tlylt · 2023-03-26T01:33:17Z

Almost there now, let's try to get this one merged soon

jovyntls

Just a few more @ong6 !

packages/core/src/Layout/Layout.ts

packages/core/src/Layout/LayoutManager.ts

jovyntls · 2023-03-30T06:54:54Z

packages/core/src/Page/index.ts

+    const doesLayoutHavePageNav = this.pageConfig.layoutManager.layoutHasPageNav(
+      this.layout ?? LAYOUT_DEFAULT_NAME);


Just checking, is there a change in behaviour here (and the other uses of ?? LAYOUT_DEFAULT_NAME below)?

Thanks for the catch. I've adjusted the code for this instance as it may affect the behavior. However, for the instances below, there is no change in the intended behavior as LAYOUT_DEFAULT_NAME is the fallback for those cases regardless.

packages/core/src/Layout/LayoutManager.ts

jovyntls

Hi @ong6, I think you need to fix the rebase - the tests are failing because your .gitignore/.eslintignore are not ignoring the files that they should, probably because of an error when you rebased

Please rebase carefully! Your rebase deleted all of the changes from master in the .gitignore and .eslintignore.

jovyntls · 2023-04-02T02:28:03Z

packages/core/src/Layout/LayoutManager.ts

    }

-    return this.layouts[name].insertPage(pageContent, pageNav, pageIncludedFiles);
+    return this.layouts[name].insertPage(pageContent, pageNav, new Set<string>(pageIncludedFiles));


Why do we create a new instance of pageIncludedFiles here? Is it to avoid passing by reference?

jovyntls · 2023-04-02T02:46:53Z

packages/core/src/Page/index.ts

+    const doesLayoutHavePageNav = this.layout ? this.pageConfig.layoutManager.layoutHasPageNav(
+      this.layout) : false;


I think we can avoid all of these typing issues with this.layout, and the additions of ?? LAYOUT_DEFAULT_NAME below, by asserting that the layout attribute exists with a ! operator.
You'll need to modify this too (just delete the question mark)

Feel free to trace to verify that this is safe!

ong6 · 2023-04-11T09:28:06Z

Will create new PR for this

ong6 changed the title ~~Migrate Layout File to Typescript~~ [WIP] Migrate Layout File to Typescript Feb 13, 2023

ong6 marked this pull request as ready for review February 13, 2023 10:55

ong6 changed the title ~~[WIP] Migrate Layout File to Typescript~~ Migrate Layout File to Typescript Feb 13, 2023

ong6 force-pushed the ts-migrate-layout branch 2 times, most recently from 88c4681 to 754fd5e Compare February 13, 2023 12:00

ong6 requested a review from jovyntls February 13, 2023 12:11

jovyntls suggested changes Feb 13, 2023

View reviewed changes

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

raysonkoh reviewed Feb 19, 2023

View reviewed changes

.eslintignore Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

ong6 force-pushed the ts-migrate-layout branch from fb51385 to 7a54356 Compare March 16, 2023 12:07

ong6 requested a review from jovyntls March 16, 2023 12:09

jovyntls suggested changes Mar 17, 2023

View reviewed changes

ong6 force-pushed the ts-migrate-layout branch from 7a54356 to 6236b99 Compare March 17, 2023 02:56

ong6 requested a review from jovyntls March 17, 2023 02:56

ong6 force-pushed the ts-migrate-layout branch from 6236b99 to 648246d Compare March 17, 2023 04:23

ong6 requested a review from raysonkoh March 19, 2023 17:56

jovyntls approved these changes Mar 20, 2023

View reviewed changes

packages/core/src/Layout/LayoutManager.ts Outdated Show resolved Hide resolved

ong6 force-pushed the ts-migrate-layout branch from 648246d to 4f457f7 Compare March 20, 2023 09:05

jovyntls added this to the v4.1.1 milestone Mar 20, 2023

raysonkoh approved these changes Mar 20, 2023

View reviewed changes

jovyntls reviewed Mar 21, 2023

View reviewed changes

ong6 force-pushed the ts-migrate-layout branch 5 times, most recently from a87e0cc to fa7caf3 Compare March 23, 2023 07:39

jovyntls suggested changes Mar 23, 2023

View reviewed changes

.eslintignore Outdated Show resolved Hide resolved

ong6 force-pushed the ts-migrate-layout branch 2 times, most recently from d11d771 to a56de11 Compare March 25, 2023 18:02

ong6 requested a review from jovyntls March 25, 2023 18:02

jovyntls suggested changes Mar 26, 2023

View reviewed changes

lhw-1 mentioned this pull request Mar 26, 2023

[WIP] Migrate Site/index.js to TypeScript #2210

Closed

11 tasks

ong6 force-pushed the ts-migrate-layout branch 4 times, most recently from de51a91 to 231d39c Compare March 29, 2023 03:22

jovyntls suggested changes Mar 29, 2023

View reviewed changes

packages/core/src/Layout/Layout.ts Outdated Show resolved Hide resolved

packages/core/src/Layout/LayoutManager.ts Show resolved Hide resolved

ong6 force-pushed the ts-migrate-layout branch 4 times, most recently from 19d94b4 to c71bbf9 Compare March 29, 2023 17:11

jovyntls reviewed Mar 31, 2023

View reviewed changes

ong6 force-pushed the ts-migrate-layout branch from c71bbf9 to ff8d20c Compare March 31, 2023 15:13

Rename Layout to Typescript

19f969d

ong6 force-pushed the ts-migrate-layout branch from 216c6d3 to 00ada70 Compare March 31, 2023 15:59

Adapt Layout to TypeScript

f11595a

ong6 force-pushed the ts-migrate-layout branch from 00ada70 to f11595a Compare March 31, 2023 15:59

jovyntls suggested changes Apr 2, 2023

View reviewed changes

lhw-1 removed this from the v4.1.1 milestone Apr 3, 2023

tlylt marked this pull request as draft April 10, 2023 10:37

ong6 closed this Apr 11, 2023

tlylt mentioned this pull request Jul 29, 2023

Fully adapt core package and associated tests to TypeScript #1913

Closed

91 tasks

This was referenced Mar 9, 2024

Migrate layout to Typescript #2447

Closed

Migrate layout to Typescript #2452

Merged

		const doesLayoutHavePageNav = this.pageConfig.layoutManager.layoutHasPageNav(
		this.layout ?? LAYOUT_DEFAULT_NAME);

		const doesLayoutHavePageNav = this.layout ? this.pageConfig.layoutManager.layoutHasPageNav(
		this.layout) : false;

Conversation

ong6 commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jovyntls left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

raysonkoh left a comment

Choose a reason for hiding this comment

Uh oh!

raysonkoh commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

tlylt commented Mar 26, 2023

Uh oh!

jovyntls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jovyntls Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

ong6 Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jovyntls left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jovyntls Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

jovyntls Apr 2, 2023

Choose a reason for hiding this comment

Uh oh!

ong6 commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ong6 commented Feb 13, 2023 •

edited

Loading

jovyntls left a comment •

edited

Loading

raysonkoh commented Mar 20, 2023 •

edited

Loading

jovyntls left a comment •

edited

Loading