-
Notifications
You must be signed in to change notification settings - Fork 10
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 new blocks supporting Pagination and Widgets #28
Add new blocks supporting Pagination and Widgets #28
Conversation
Thanks for opening this PR @chrispenny, it looks like a really nice addition! I'm going to get the |
Nice! Thank you for the speedy feedback, @muskie9 ! I will review the tests also. I would have expected this to be fine for PHP >=7.1, so, I think maybe I've just messed something up for when these tests are run in isolation (rather than from within a project) |
6a5ef30
to
a0df09a
Compare
Yea, it's not the type hinting (I just tried removing it all, but the PHP I was using |
92690d9
to
f68f1be
Compare
fe8bf83
to
bb8ae3c
Compare
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
============================================
+ Coverage 83.72% 90.82% +7.10%
- Complexity 12 41 +29
============================================
Files 1 2 +1
Lines 43 109 +66
============================================
+ Hits 36 99 +63
- Misses 7 10 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bb8ae3c
to
23cd8b1
Compare
@muskie9 sorry for all the commit noise. After all of that, it turns out that I'm just a goose. I forgot to add Tests are now passing - I will just see about increasing the coverage a bit more. |
Now worries, glad to you got it sorted ;) |
I'm hoping to review this over the next couple of days. Just curious, do you have this implemented on a public URL somewhere? |
@muskie9 the project goes live on Monday - but, I have a feeling that they won't have any blog posts ready for then. It's also worth me mentioning that the project is not using this fork, but, this contribution is essentially a copy/paste of those classes, but with configuration options added. |
Anything I can do to help this one along? |
@muskie9 any update on this one? Is the feature still desired as part of this module, or would it be preferred that I split this into a separate module? |
Hi @chrispenny, Sorry for this falling off our radar. We did briefly look into the PR but weren't able to put the amount of attention to it as it deserved. We really like the concept, but the project we planned on testing with went in a different direction unfortunately. I'd say at this point splitting it would get it into a stable state sooner with a release. |
62c5333
to
cabf598
Compare
It's been quite a while since I've had a project that needs to mix Blog posts within an Elemental Area, but thankfully an opportunity has just presented itself. I plan to implement this branch as part of that development. I'll see how it goes, and once it's been in PROD (and stable) for a while, I'll look to merge this in. That said, there seem to be some PHPUnit errors that I don't think are related to my changes. PHP Fatal error: Uncaught Error: Call to undefined function xdebug_disable() in /home/runner/work/silverstripe-elemental-blog/silverstripe-elemental-blog/vendor/phploc/phploc/src/CLI/Application.php:112 |
@chrispenny I think you're absolutely right. I know @jsirish has done some work with the github CI in #31 and targeted master. I realized I don't think I merged 2.1 up and retargeted as I called out in an earlier comment. I'll work on getting that sorted tonight. One thing I should call out is the change Jason is making is for the upcoming 4.10 release as it relates to the phpunit version. We can hold off with merging that PR for now if you think that would cause problems with getting this across the line. We just want to make sure we're getting our modules updated for the new phpunit support as well as PHP 8. We just happened to start with this one ;) |
@chrispenny I've re-worked the CI to remove the irrelevant parts if you want to rebase. Tests should hopefully pass then. |
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've gone through installing with various configurations, and it took me a second to realize how the defaults matched up to using the pagination and widget blocks but once it clicked it all made sense. I think this can get merged once we know the rebase is all good.
One thing I did notice with my testing is based on the widget's dev requirement, it will want to pull in 2.2.0 which already requires framework ^4.10. That could cause some pain for devs wanting this feature but not ready to upgrade to the RC. That being said, I'm ok with helping that upgrade to 4.10 along if it means merging this and requiring projects get updated.
Thanks, @muskie9! I would say the upgrade for PHPUnit should take priority over this. I'll be happy to rebase and update my unit tests once that happens - I don't think it will be an issue. I will check out the widget requirements as well, and see what can be done. I'm not really in a rush to merge this atm. For now, I'm pretty happy with just keeping my fork up to date with any changes the team makes and seeing how this current project implementation goes. |
99c1235
to
5078364
Compare
d702bd0
to
9e629e7
Compare
@chrispenny let me know if you're still actively working on this/this is a need. We're actively working on providing resources to a number of our (elemental) modules and this is right up there. We did play around with the concept and we do see the value, so we'd love to really work out how this can be brought home and merged in. |
@muskie9-opensource actually, this is ready to go. I rebased it a little while back for one of our projects on PHP 8.1, so it's been tested there as well. I meant to come back and hit the [go] button, but then I think I got distracted fighting fires. Would you like to do the honours? |
@chrispenny no need to wait for me ;) I won't get around to this right away as we're sorting through (all of) our current modules, figuring out which should receive priority updates. This is one that has already made the "actively support" list, but our immediate focus will be bringing others up to a supported standard. So excited to see this in the module and would love to get a discussion going on how this could be expanded to a UX without a page reload. That's a whole other can of worms though. |
@muskie9-opensource I was thinking of tagging this as |
That sounds perfect @chrispenny! |
Purpose
To add support for more of the standard Blog features, including:
New block
The main block that has been added is
ElementBlogOverview
. This block has the ability to display all of the original features of the Blog module'sLayout
template. Including:It is highly configurable, so it should be very easy to customise what fields are available to CMS users, and also what aspects of the features are displayed (EG: You can disable Pagination and/or Widgets).
Example of the Overview block being used to display paginated Blog Posts, Pagination, and Widgets - with Content blocks on either side.
Additional blocks
In case developers/users don't want to tie the Posts, Pagination, and Widgets together in a single block, two additional blocks have been added:
ElementBlogPagination
ElementBlogWidgets
These are actually just extended classes of the
Overview
block, but they have different default configuration settings, and they have simplified default templates.The reason they extend the
Overview
block is mostly because they have a tonne of the same concerns, but, also because it allows significantly more flexibility if a developer needs other information about the Blog (EG: You might want to get the Post count, even for your Widgets area).Example of the 3 blocks being used separately on the same page with other Content blocks between them.