Skip to content
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

Migrate layout to Typescript #2447

Closed
wants to merge 3 commits into from
Closed

Conversation

yiwen101
Copy link
Contributor

@yiwen101 yiwen101 commented Mar 9, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

work on #1913. Credit to #2164 for some of the works. Did not commit to #2164 to avoid the git rebase issues currently haunting it.

Overview of changes:
Update the use of layout manager in Site and Page;
Added two more types: LayoutConfig and PageNjkAssets to avoid any
Fix some nits/bugs? along the way:
in Site:
await this.layoutManager.updateLayouts(filePath)
=>
await this.layoutManager.updateLayouts(filePathArray)

and in Layout.ts

generateDependencies(pageSources.getDynamicIncludeSrc(),
this.includedFiles)
=>
await this.config.externalManager.generateDependencies(
pageSources.getDynamicIncludeSrc(),
this.includedFiles,
this.layoutUserScriptsAndStyles);
}

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Migrate layout to Typescript

Checklist: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.95%. Comparing base (1c01cfb) to head (3d24b85).

Files Patch % Lines
packages/core/src/Layout/Layout.ts 46.15% 7 Missing ⚠️
packages/core/src/Layout/LayoutManager.ts 50.00% 7 Missing ⚠️
packages/core/src/Page/index.ts 25.00% 3 Missing ⚠️
packages/core/src/Site/index.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2447      +/-   ##
==========================================
+ Coverage   48.92%   48.95%   +0.02%     
==========================================
  Files         124      124              
  Lines        5245     5248       +3     
  Branches     1110     1110              
==========================================
+ Hits         2566     2569       +3     
- Misses       2371     2374       +3     
+ Partials      308      305       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yucheng11122017
Copy link
Contributor

Hi @yiwen101 sorry I won't have time to check this tonight but migrating TS PRs should only have 2 commits as per DG
Could you follow the instructions there? Thanks!

@yiwen101
Copy link
Contributor Author

yiwen101 commented Mar 10, 2024

Hi @yiwen101 sorry I won't have time to check this tonight but migrating TS PRs should only have 2 commits as per DG
Could you follow the instructions there? Thanks!

Haha, sure; Will open a new PR with the same modification ASAP.

Actually the third commit is added right during our meeting today;

Unsure of whether I could press "update this branch" instead of amend, force push (as in DG) to merge the master branch, I clarify during the meeting by sharing my screen pointing to this PR and the DG page. I was told that I could simply press "update this branch" :(

Sorry for the trouble and appreciate your help in pointing me to the exact part in the DG.

@yiwen101 yiwen101 closed this Mar 10, 2024
@yiwen101 yiwen101 mentioned this pull request Mar 10, 2024
14 tasks
@kaixin-hc
Copy link
Contributor

My bad @yiwen101 ! While generally you can press update this branch, you need to take particular care for typescript migration due to our established workflow, and I didn't realize the context when I gave that advice

(I think if you had clicked update this branch with rebase it would have been okay)

Normally, it doesn't matter because PRs are squash-merged, but in this case it does since we're trying to preserve the doc history.

@yiwen101
Copy link
Contributor Author

My bad @yiwen101 ! While generally you can press update this branch, you need to take particular care for typescript migration due to our established workflow, and I didn't realize the context when I gave that advice

(I think if you had clicked update this branch with rebase it would have been okay)

Normally, it doesn't matter because PRs are squash-merged, but in this case it does since we're trying to preserve the doc history.

Noted, thank you for the detailed explaination!

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.

3 participants