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

Jump to bottom

Isolate course blocks #3885

Merged
merged 9 commits into from Jan 15, 2021
Merged

Isolate course blocks #3885

merged 9 commits into from Jan 15, 2021

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Jan 11, 2021 •
edited

Part of #3821

Changes proposed in this Pull Request

  • This PR purpose isolating the course blocks, so we can introduce the lesson blocks separately.
  • This also contains some small cleanups and fixes.

Testing instructions

  • Create a course with blocks, and make sure it continues working properly.

@renatho renatho added this to the 3.8.0 milestone Jan 11, 2021
@renatho renatho requested a review from a team January 11, 2021 20:52
@renatho renatho self-assigned this Jan 11, 2021
@renatho renatho marked this pull request as draft January 11, 2021 21:25
yscik
yscik reviewed Jan 11, 2021
View reviewed changes
assets/blocks/blocks-setup.js Outdated

import { SenseiIcon } from '../icons';

const blocksSetup = ( blocks ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like registerSenseiBlocks?

Copy link
Contributor Author

@renatho renatho Jan 12, 2021 •
edited

Choose a reason for hiding this comment

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

I used a more generic name because we were also setting the category icon, and we can to other stuff there, but I think this name works well too.

Updated in b09d1ef

includes/blocks/class-sensei-course-blocks.php Outdated
@@ -86,7 +83,7 @@ public function enqueue_block_editor_assets() {
return;
}

Sensei()->assets->enqueue( 'sensei-blocks', 'blocks/index.js', [], true );
Sensei()->assets->enqueue( 'sensei-course-blocks', 'blocks/course-blocks.js', [], true );
Copy link
Contributor

Choose a reason for hiding this comment

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

We could go with single-course for the name, if it makes sense that these are grouped based on where they need to be loaded. The CSS files are named like that right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review, @yscik!

I got your point, and I think it makes sense. But I think we could make this change in a future PR (maybe when we need that), just to avoid conflicts with this now: #3885 (comment)

It's because we'd need also to move this file out of the blocks folder to make sense.

@gikaragia
Copy link
Contributor

Hey @renatho I have done some similar work in #3849. Is it possible to consolidate these two or prioritize the other?

@renatho renatho force-pushed the change/isolate-course-blocks branch from 19ed087 to b579fbb Compare January 12, 2021 15:12
@renatho renatho marked this pull request as ready for review January 12, 2021 15:18
@renatho renatho marked this pull request as draft January 12, 2021 15:18
@renatho renatho force-pushed the change/isolate-course-blocks branch from b579fbb to eae2c2c Compare January 12, 2021 17:07
@renatho renatho force-pushed the change/isolate-course-blocks branch from 91a2e45 to cea5369 Compare January 12, 2021 17:35
@renatho
Copy link
Contributor Author

renatho commented Jan 12, 2021 •
edited

Hey @renatho I have done some similar work in #3849. Is it possible to consolidate these two or prioritize the other?

Hey @gkaragia! Thank you for looking! As we talked, I took a look, and I did some things to make it easier:

  • I removed my change about the JS file, and I did a cherry-pick on yours: 12f02ff
  • About the $sensei variable in some classes, I think we implemented the same (the only difference is that I removed a space before a period in a comment). I tried to cherry-pick yours, but yours had some other things, so I kept it untouched, but considering the change is only the space, I believe we won't have problems while merging. :)

Anything else, let me know.

@renatho renatho marked this pull request as ready for review January 12, 2021 17:49
@renatho renatho requested review from yscik and a team January 12, 2021 17:49
This was referenced Jan 12, 2021
Merged
Merged
gikaragia
gikaragia approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

Looks good, works well!

jom
jom approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Looks like good changes.

@renatho renatho merged commit d324b93 into feature/lessons-block Jan 15, 2021
@m1r0 m1r0 deleted the change/isolate-course-blocks branch November 24, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jom jom jom approved these changes

@gikaragia gikaragia gikaragia approved these changes

@yscik yscik Awaiting requested review from yscik

Assignees

@renatho renatho

Labels
[Type] Enhancement
Projects
None yet
Milestone
3.8.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@renatho @gikaragia @jom @yscik