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

Convert Template function to class #2148

Merged
merged 6 commits into from
Feb 12, 2023
Merged

Conversation

lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Feb 8, 2023

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:

Overview of changes:

Resolves #2147.

Converts the Template function inside packages/core/template/template.js into a class.

This also simplifies the migration process for template.js into TypeScript in #2143.

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Convert Template function to class


Checklist: ☑️

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

@lhw-1 lhw-1 changed the title [WIP] Convert Template function to class Convert Template function to class Feb 8, 2023
@lhw-1 lhw-1 requested a review from a team February 9, 2023 08:30
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Interesting changes proposed!

Curious if there are opportunities here to improve the code further. For example, I wonder whether template.js really needs to be in this folder, or perhaps it is more suited in the Site folder. What do you think?

packages/core/template/template.js Outdated Show resolved Hide resolved
packages/core/template/template.js Outdated Show resolved Hide resolved
packages/core/template/template.js Outdated Show resolved Hide resolved
@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 9, 2023

Curious if there are opportunities here to improve the code further. For example, I wonder whether template.js really needs to be in this folder, or perhaps it is more suited in the Site folder. What do you think?

On one hand, I think keeping template.js in the template folder makes sense since this is acting as a sort of "interface" for other files to create and initialize a template (though only Site/index.js is doing that right now).

But on the other hand, I also see the benefit in moving it over to the Site folder to (1) keep the JavaScript / TypeScript files all in once place (within src), and (2) since Site contains the files for initializing a MarkBind site / converting to a MarkBind site, it makes sense that we should have the Template class within this folder. And of course, the only file using the Template function / class right now is Site/index.js, which is one more point towards moving template.js to the Site folder.

Yes, I think moving it over may make more sense than keeping it where it currently is.

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 10, 2023

Update in 931271a:

template.js has been moved from packages/core/template/ to packages/core/src/Site/ as per the suggestion from @tlylt & the following discussion.

As the template files reside in another directory outside packages/core/src/Site, an additional correction to the path needed to be declared here. I am wondering if it would be a good idea to additionally move all the template files into the packages/core/src/Site folder to make it more direct, or if it would make more sense to keep the template in a separate folder as is currently being done. I would appreciate thoughts on this matter as I am uncertain if this makes sense 👍

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

I am wondering if it would be a good idea to additionally move all the template files into the packages/core/src/Site folder to make it more direct, or if it would make more sense to keep the template in a separate folder as is currently being done. I would appreciate thoughts on this matter as I am uncertain if this makes sense 👍

I think leaving them there is fine, because of how they are used and also that in the future, that folder can potentially grow further in the event that we increase the number of templates supported.

As for the shift of template.js I was wondering if that causes any cyclic dependency, which doesn't seem to be the case so we should be good.

Lastly, I don't like how we have a "pass-through" method initSite that simply calls new Template().init, which feels like a red flag in the class design. Might need further refactoring in the future.

LGTM.

@tlylt tlylt added this to the 4.0.3 milestone Feb 10, 2023
@lhw-1 lhw-1 removed the request for review from a team February 10, 2023 15:30
@tlylt tlylt merged commit 62e8988 into MarkBind:master Feb 12, 2023
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.

Convert template.js to a class
2 participants