dynamic / silverstripe-elemental-blog

Display a list of the most recent posts of a specific blog
BSD 3-Clause "New" or "Revised" License
1 stars 10 forks source link

Add new blocks supporting Pagination and Widgets #28

Closed chrispenny closed 2 years ago

chrispenny commented 4 years ago

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's Layout 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.

Screen Shot 2020-01-29 at 2 34 57 PM

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:

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.

Screen Shot 2020-01-29 at 2 33 52 PM
muskie9 commented 4 years ago

Thanks for opening this PR @chrispenny, it looks like a really nice addition! I'm going to get the master branch caught up to 2.1 and will re-target this PR against master as this would require a new feature release (2.2.0). I'll also remove PHP 7.0/7.1 from the travis config as well, I noticed some syntax is failing and they have reached EOL.

chrispenny commented 4 years ago

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)

chrispenny commented 4 years ago

Yea, it's not the type hinting (I just tried removing it all, but the PHP >=7.1 tests still failed).

I was using silverstripe/blog 3.5.0 during development, so I'll try rolling back to 3.0 and see how that goes.

codecov[bot] commented 4 years ago

Codecov Report

Merging #28 (9e629e7) into master (824e541) will increase coverage by 7.10%. The diff coverage is 95.45%.

@@             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     
Flag Coverage Δ
unittests 90.82% <95.45%> (+7.10%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Elements/ElementBlogOverview.php 95.45% <95.45%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 824e541...9e629e7. Read the comment docs.

chrispenny commented 4 years ago

@muskie9 sorry for all the commit noise. After all of that, it turns out that I'm just a goose. I forgot to add ElementalPageExtension::class in $required_extensions for Page 🤦‍♂

Tests are now passing - I will just see about increasing the coverage a bit more.

muskie9 commented 4 years ago

Now worries, glad to you got it sorted ;)

muskie9 commented 4 years ago

I'm hoping to review this over the next couple of days. Just curious, do you have this implemented on a public URL somewhere?

chrispenny commented 4 years ago

@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.

chrispenny commented 4 years ago

Anything I can do to help this one along?

chrispenny commented 3 years ago

@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?

muskie9 commented 3 years ago

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.

chrispenny commented 2 years ago

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
muskie9 commented 2 years ago

@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 ;)

muskie9 commented 2 years ago

@chrispenny I've re-worked the CI to remove the irrelevant parts if you want to rebase. Tests should hopefully pass then.

chrispenny commented 2 years ago

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.

muskie9-opensource commented 2 years ago

@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.

chrispenny commented 2 years ago

@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?

muskie9-opensource commented 2 years ago

@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.

chrispenny commented 2 years ago

@muskie9-opensource I was thinking of tagging this as 2.3 (with a new 2.3 branch as well). Was that what you were thinking?

muskie9-opensource commented 2 years ago

That sounds perfect @chrispenny!