-
Notifications
You must be signed in to change notification settings - Fork 199
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 Restricted Course Content block #3849
Conversation
@@ -0,0 +1,67 @@ | |||
<?php |
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.
Written this class as part of my investigation of adding the blocks to the lesson page. Happy to remove it since it is unused.
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.
I think we could remove this since it's being worked in the other PRs of the lesson blocks :)
15291b3
to
98c0273
Compare
Dropping a quick note that this one should go through a design review before being merged. Making a note to do that this week. |
Nice! A few things that would be good to have in this:
|
Design-wise, I think the main thing here is a way to indicate in the editor when content is restricted. This is kind of a wider issue of making a parent block more visible, that also came up with the course outline block, and will hopefully be handled by Gutenberg at some point. Until then, we should roll our own solution, but it's worth a dedicated PR. The core Gallery block already selects both the container an the inner block: Jetpack's Repeat Visitor block has a note, but it's unclear where the block ends: I also explored some options in Conditional Sections for Course Enrollment Blocks (p6rkRX-1Dd-p2): |
e575da4
to
9afe9e7
Compare
|
Great ideas! Added transforms and alignment options (following the group block). |
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.
Code is looking good, just a few notes:
type: 'block', | ||
isMultiBlock: true, | ||
blocks: [ '*' ], | ||
__experimentalConvert: ( blocks ) => { |
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.
There is a transform(attributes, innerBlocks)
function as a stable API, any reason this one is used?
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.
Yes transform includes the attributes and inner blocks only but we need the plugin name which is provided by convert. It also supports better multiple selections. It seems to be the plan to deprecate transform in favor of convert so I thought that it is safe enough to use.
title: __( 'Unenrolled Content', 'sensei-lms' ), | ||
description: __( | ||
'Content inside this container block will be displayed to unenrolled users only.', |
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.
I don't think we use unenrolled
anywhere, something like ..users not enrolled
might sound better.
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.
6dbfd63
to
23072f1
Compare
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.
Cool! I think this feature will be very useful! Did you also think about the option to create a single block with multiple states as a setting (enrolled, unenrolled, course completed...)?
For the design meeting, maybe would be nice to think about the unenrolled icon too. The open padlock gives a sensation that it is unlocked for any user (although the title says clearly the block purpose).
cc @donnapep
Yep I considered it but thought that separate blocks are probably more easy to understand. |
While the use case makes sense, it'd be better to think of this as an implementation that shares paradigms to similar cases like Pay Wall in Dotcom. @alexsanford worth connecting with @fditrapani and @artpi, and more recently @apeatling Additionally, it's more natural to think of the premium/extra content as the block to be added, rather than creating a paradox of rolled/unrolled. So the creator has the clarity that in order to add special content, they need to add the Premium (or whatever the right term is) block. |
Good point, the "Premium Content" block on dotcom seems like a good place from which to draw inspiration.
I think both paradigms are needed. A concrete example that came up when building WP Courses was the need to have sales copy on the Course page that only displays for non-enrolled users. Note that the Premium Content block has this capability; the "Visitor View" is a fully editable container. Perhaps it would make sense to do something similar, having a "Visitor View" and "Learner View"? |
d3605de
to
18eb971
Compare
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.
It works well, and from the code perspective, it looks good to me!
I added a few comments, but nothing critical.
Before merging, it worths it to wait for UX/product feedback.
I investigated to possibly add the blocks to the single lesson page too. Currently in the single lesson template we don't show the content at all when the user is not enrolled. For this reason we should probably wait until we drop the single lesson template to see if it fits to add these blocks.
Nice! When we can, I think it would be nice to use this block in the lessons too, and maybe in the quizzes.
@@ -0,0 +1,67 @@ | |||
<?php |
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.
I think we could remove this since it's being worked in the other PRs of the lesson blocks :)
254ed67
to
216adca
Compare
216adca
to
6f04c7a
Compare
34a0307
to
8a91225
Compare
There were Peter's comments about the popover but these should be solved as with the latest rebase I switched to using the common one introduced in #3904. |
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.
Tested and works well, code LGTM! Note that I used the Github feature to merge the latest from feature/lessons-block
into this branch.
Changes proposed in this Pull Request
Update:
Second Update:
Testing instructions