-
Notifications
You must be signed in to change notification settings - Fork 133
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
WIP: Add support for memberspace #712
Conversation
💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides it hardcoding a fragment into another fragment this needs docs and a test page.
If this seems like non doable we can push this to the backlog and I'll cancel memberspace.
@@ -172,6 +172,7 @@ | |||
looking from the page directory or file to it's parents and so on. */}} | |||
{{- $page_scratch.Set "page_fragments" (slice) -}} | |||
{{- $page_scratch.Set "page_config" (slice) -}} | |||
{{- $page_scratch.Set "memberspace" nil -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a global variable for a vendor specific fragment seems problematic.
@@ -81,6 +81,13 @@ | |||
</a> | |||
</li> | |||
{{- end -}} | |||
{{- end -}} | |||
{{- with ($page_scratch.Get "memberspace") }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a vendor specific fragment into another fragments code seems problematic.
We should inject it from a memberspace fragment or at least make this injection more generic.
As it currently stands this seems too bundled.
Currently I can't think of any approach that would be less complex. I would love to hear ideas on the matter. Maybe remove the fragment and make it a site configuration, applied to the baseof template? |
I'm curious, it is like a paywall ? |
Memberspace is a paywall service especially for membership use cases. They have different modes of security. Simplest will remove content until payment via JS, one will add the content in via JS from their DB after payment/login. Authentication is therefore done on the memberspace side of things. Link to service: https://memberspace.com |
I didnt knew it was a SaaS |
Most likely depends on the mode used. |
Let's shelf this for now. We first need a better idea around provider fragments and also can't add things globally for a fragment. Possible option for injecting something like this is the fragments JS or our event framework. Let's discuss this separately. |
What this PR does / why we need it:
Add support for memberspace
Which issue this PR fixes:
fixes #631
Special notes for your reviewer:
The implementation works with a fragment that you can add to _global or any page or section globals and in turn nav fragment in those pages will display a login link. I didn't add anymore customizations since I'm not even sure if this is the best approach. My assumptions is that the user would create a memberspace.md file in _global with following content:
And then the nav fragment will display a link to Login/Signup page in Memberspace. It would be recommended that the button Memberspace provides be disabled from their website. But we can also move the "go to top" button as we discussed.
If you have any other approach in mind please let me know and if everything is okay I can add documentation and we can move forward with the merge.
Release note: