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

Add new blocks supporting Pagination and Widgets #28

Merged

Conversation

chrispenny
Copy link
Collaborator

Purpose

To add support for more of the standard Blog features, including:

  • Pagination
  • Widgets

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:

  • Title (including Category/Archive/etc titles)
  • Content
  • Blog Posts
  • Pagination
  • Widgets (if the addon as been added and enabled)

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:

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

Screen Shot 2020-01-29 at 2 33 52 PM

@muskie9
Copy link
Member

muskie9 commented Jan 29, 2020

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.

@muskie9 muskie9 self-requested a review January 29, 2020 03:38
@muskie9 muskie9 added this to the 2.2.0 milestone Jan 29, 2020
@chrispenny
Copy link
Collaborator Author

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 chrispenny force-pushed the feature/pagination-widget-support branch from 6a5ef30 to a0df09a Compare January 29, 2020 18:43
@chrispenny
Copy link
Collaborator Author

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.

@chrispenny chrispenny force-pushed the feature/pagination-widget-support branch from 92690d9 to f68f1be Compare January 29, 2020 19:34
@chrispenny chrispenny changed the title Add new blocks supporting Pagination and Widgets WIP: Add new blocks supporting Pagination and Widgets Jan 29, 2020
@chrispenny chrispenny force-pushed the feature/pagination-widget-support branch 3 times, most recently from fe8bf83 to bb8ae3c Compare January 29, 2020 20:56
@codecov
Copy link

codecov bot commented Jan 29, 2020

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%) ⬆️

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 chrispenny force-pushed the feature/pagination-widget-support branch from bb8ae3c to 23cd8b1 Compare January 29, 2020 21:01
@chrispenny
Copy link
Collaborator Author

@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
Copy link
Member

muskie9 commented Jan 29, 2020

Now worries, glad to you got it sorted ;)

@muskie9
Copy link
Member

muskie9 commented Jan 29, 2020

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
Copy link
Collaborator Author

@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 chrispenny changed the title WIP: Add new blocks supporting Pagination and Widgets Add new blocks supporting Pagination and Widgets Jan 30, 2020
@chrispenny
Copy link
Collaborator Author

Anything I can do to help this one along?

@chrispenny
Copy link
Collaborator Author

@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
Copy link
Member

muskie9 commented Feb 23, 2021

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 chrispenny force-pushed the feature/pagination-widget-support branch from 62c5333 to cabf598 Compare January 5, 2022 20:31
@chrispenny
Copy link
Collaborator Author

chrispenny commented Jan 5, 2022

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
Copy link
Member

muskie9 commented Jan 6, 2022

@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 muskie9 changed the base branch from 2.1 to master January 6, 2022 02:29
@muskie9
Copy link
Member

muskie9 commented Jan 6, 2022

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

Copy link
Member

@muskie9 muskie9 left a 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.

@chrispenny
Copy link
Collaborator Author

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.

@chrispenny chrispenny force-pushed the feature/pagination-widget-support branch from 99c1235 to 5078364 Compare January 6, 2022 18:41
@chrispenny chrispenny force-pushed the feature/pagination-widget-support branch from d702bd0 to 9e629e7 Compare July 25, 2022 20:31
@muskie9-opensource
Copy link

@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
Copy link
Collaborator Author

@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
Copy link

@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 chrispenny merged commit 77f1cf6 into dynamic:master Sep 19, 2022
@chrispenny
Copy link
Collaborator Author

@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
Copy link

That sounds perfect @chrispenny!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants