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

Add option to configure filter names for url plugin #149

Merged

Conversation

tobiasschmidt89
Copy link
Contributor

@tobiasschmidt89 tobiasschmidt89 commented Dec 3, 2021

  • Update url plugin to accept an option to configure url and htmlUrl filter names.
  • Implement tests for default filter names and configured filter names including two test pages.
  • I am unsure about the name for the options as no other plugin has two filter names. Other plugins could therefore use name as the property. I propose to use options.names.filterDefaultName as the pattern in this case. e.g. options.names.url

- Update url plugin to accept option to configure url and htmlUrl filter names.
- Implement test for default filter names and configured filter names including two test pages.
- I am unsure about the name for the options as no other plugin has two filter names. Other plugins could therefore use name as the property.
@oscarotero
Copy link
Member

Many thanks @tobiasschmidt89. I think the implementation is correct, I just have a couple of points:

  1. Plugins loaded by default (like this one) must be configured in the site creation function, using the second argument of lume() function. For example:
const  url = {
  names: { url: "urlify", htmlUrl: "htmlUrlify" }
};

const site = lume(
  { location: new URL("https://example.com") },
  { url }
);

See the mod.ts file. You need to include the url Options in the PluginsOptions interface and pass the values here.

  1. After done the step 1, in the test we need to update the getSite function to accept default plugins configuration as the second argument.

tests/url.test.ts Outdated Show resolved Hide resolved
@tobiasschmidt89
Copy link
Contributor Author

tobiasschmidt89 commented Dec 4, 2021

@oscarotero understood. I will make an update to include the options in the default plug-in options in mod.ts and I will then adjust the tests to define the options in the getSite function itself.

- Update mod.ts with url plugin options interface.
- Update mod.ts to pass default plugin options to url plugin.
- Update my new tests to pass options in default plugin options to the test lume instance.
- Update getSite test util function to accept and pass on default plugin options to the test lume instance. For now I used type any for the default plugin options.
- To discuss if it makes sense to put the default plugin options interface inside e.g. the plugin folder and export it so that it can be used in both the mod.ts and getSite test function (similar to the site options) or if we can export plugin options interface from mod.ts (which I think is not nice as it might indicate that it is ok to use it by users).
@tobiasschmidt89
Copy link
Contributor Author

I pushed a new commit addressing the issues:

Update tests to use default plugin options in mod.ts.

  • Update mod.ts with url plugin options interface.
  • Update mod.ts to pass default plugin options to url plugin.
  • Update my new tests to pass options in default plugin options to the test lume instance.
  • Update getSite test util function to accept and pass on default plugin options to the test lume instance. For now I used type any for the default plugin options.
  • To discuss if it makes sense to put the default plugin options interface inside e.g. the plugin folder and export it so that it can be used in both the mod.ts and getSite test function (similar to the site options) or if we can export plugin options interface from mod.ts (which I think is not nice as it might indicate that it is ok to use it by users).

@oscarotero oscarotero self-requested a review December 4, 2021 17:48
@oscarotero
Copy link
Member

Thanks!

@oscarotero oscarotero merged commit ccb6b70 into lumeland:master Dec 4, 2021
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