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

feature/enhance-pattern-matching #11

Open
wants to merge 4 commits into
base: 1.x-1.x
Choose a base branch
from

Conversation

alvinjohnsonso
Copy link
Collaborator

@alvinjohnsonso alvinjohnsonso commented May 4, 2022

Added tawk.to pattern matching library and applied it to the module to have a more flexible URL/path rule filtering.

image

@alvinjohnsonso
Copy link
Collaborator Author

Hi @yorkshire-pudding, hope you're well. Just wanted to ask how would one structure a backdrop module that would contain external libraries managed by composer?

The way we did this to our other modules is that we created a github actions workflow to build the composer dependencies and package the module including the vendor folder.

@yorkshire-pudding
Copy link
Collaborator

Hi @alvinjohnsonso - There is some guidance on using libraries here - https://docs.backdropcms.org/documentation/using-libraries

In general, the approach is to have everything bundles inside the module folder. You would have a subfolder (libraries) and within that a folder for each library in use (e.g. tawk-url-utils ).

Does it need to be managed by composer? Making a dependency for a module to use composer is not encouraged as it raises barriers to collaboration. While lots of people have github actions running tests on PRs, I don't think we use composer.

Taking a step back, what is the need for this additional library? I've not have anything that I haven't been able to do with the existing pattern matching.

@alvinjohnsonso
Copy link
Collaborator Author

Hi @yorkshire-pudding I see. I'll probably just follow that guide you sent. Thanks for answering my question! 😁

As for why we're making this update, we've recently developed a library that provides more flexibility on doing url/path pattern matching to filter which pages to show/hide the widget from. We thought we could also use it on most of the plugins we're integrated to.

@alvinjohnsonso alvinjohnsonso marked this pull request as ready for review May 19, 2022 10:50
@alvinjohnsonso alvinjohnsonso requested a review from GeekOfAges May 19, 2022 10:51
@alvinjohnsonso
Copy link
Collaborator Author

I made the vendor folder into libraries and didn't bother using the available library API that backdrop has. It seems that the API is only for loading css and js libraries into the module. So what I did was I just manually required the autoload.php that composer generated.

@@ -0,0 +1,7 @@
<?php
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To anyone that might ask why these composer files are included is because Backdrop has its own release workflow that retrieves and builds the module when a release is published. I looked for other modules that has external libraries being used and they all included the library within their repository. Here are some of them:

@yorkshire-pudding
Copy link
Collaborator

@alvinjohnsonso - I'm sorry but I can't see that this feature addition is required to the extent that we should increase the complexity in this way. The current pattern matching works well and I have never had any need for anything more complex to meet genuine use cases.

I'm not sure what you mean by "Backdrop has its own release workflow that retrieves and builds the module when a release is published". When Backdrop builds a module, it packages the zip, and tar.gz files and updates the .info file in the package zip file. I downloaded the file from your repo branch and was able to install using manual install from that.

I think there are too many examples in the tooltip. The config only requires paths from the root leading slash, so why have all the domains? I also don't think that a tooltip is good practice for guidance. I suggest to include in a collapsible section if necessary.

I'm afraid I cannot do a code review justice on such a massive increase in code.

@alvinjohnsonso
Copy link
Collaborator Author

Hi @yorkshire-pudding I understand. This was decided internally and I've asked my superiors to review it. Thanks for the inputs! 😁

@alvinjohnsonso alvinjohnsonso removed the request for review from yorkshire-pudding May 20, 2022 02:56
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