-
Notifications
You must be signed in to change notification settings - Fork 6
[MOOSE-336] PHP Controller Classes for Blocks #313
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
base: main
Are you sure you want to change the base?
Conversation
MlKilderkin
left a comment
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.
All good. Just some suggestions and comments
wp-content/plugins/core/src/Components/Post_Card_Controller.php
Outdated
Show resolved
Hide resolved
wp-content/plugins/core/src/Components/Post_Card_Controller.php
Outdated
Show resolved
Hide resolved
wp-content/plugins/core/src/Components/Post_Card_Controller.php
Outdated
Show resolved
Hide resolved
wp-content/plugins/core/src/Components/Related_Posts_Controller.php
Outdated
Show resolved
Hide resolved
wp-content/plugins/core/src/Components/Related_Posts_Controller.php
Outdated
Show resolved
Hide resolved
wp-content/plugins/core/src/Components/Blocks/Related_Posts_Controller.php
Show resolved
Hide resolved
dpellenwood
left a comment
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.
This is really solid, Geoff.
A couple thoughts/points for conversation on the controller approach:
- I'd like to prose that we a a card abstract class is created to house all the "default" card controller logic. Then the individual card types can use and extend that default with any unique or specific implementations.
- We've been here before. SquareOne has a very similar structure fort all its components. One of the most difficult aspects to working on a Sq1 project was that you had to jump back & forth between the core plugin and core theme to build or edit components. I'd like to propose that we move these controllers into the same folder as the render (and other files) . I could see the abstract (note above) living in the core plugin and the implementations living in the theme blocks folders.
Let me know what you think about the above.
Makes sense to me - I'd like to get some help with this though. I've reached out to Caleb to ask if he has any time to chat through this and will discuss mapping this out.
This is fair and valid. Will consider for final implementation. |
|
@dpellenwood @MlKilderkin I've overhauled this whole thing to be more inline with DRY programming & what Dave suggested as far as abstract classes. With the help of @crstauf, we decided to make the post data used in the Post & Search cards a Trait so that it can be used elsewhere if necessary. Say, like the Fellow Header block on NHC where it's not a card but still might require post data. This feels a lot better to me and should allow us to spin up new blocks slightly faster. Let me know your thoughts! |
|
cc @Vinsanity - adding you as a reviewer here as well just in case you have any opinions. |
| display: inline; | ||
| background-image: linear-gradient(currentcolor, currentcolor); | ||
| background-size: 0 0.05em; | ||
| background-size: 0 0.06em; |
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.
🗒️ Noting that this change & the hover change below it are due to weird rendering where odd numbered em values could be rendered at different heights, causing some elements to have a smaller underline than others.
| /* ----------------------------------------------------------------------- | ||
| * Post Card - Excerpt | ||
| * | ||
| * Hides content after 3 lines | ||
| * | ||
| * ----------------------------------------------------------------------- */ | ||
|
|
||
| .c-post-card__excerpt { | ||
| margin-top: 0 !important; | ||
| overflow: hidden; | ||
| display: -webkit-box; | ||
| -webkit-box-orient: vertical; | ||
| -webkit-line-clamp: 3; | ||
| } | ||
|
|
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.
🗒️ The post cards in the Moose design system in Figma now have an excerpt and it looked like the cards in the code in Moose hadn't been updated to match, so I did that as part of these updates and styled it here.
What does this do/fix?
This pull request introduces a significant architectural refactor to the block controller system, moving dynamic block data handling to dedicated PHP controller classes. This change lays the foundation for a more object-oriented, MVC-style approach, making block logic more modular, maintainable, and testable. The update also includes new abstract controller classes, several new block controller implementations, trait refactoring, and related codebase clean-up. Additionally, there are targeted CSS improvements for card components.
Core architecture and controller refactor:
Abstract_Block_ControllerandAbstract_Card_Controllerbase classes to encapsulate shared block logic and attributes, promoting code reuse and OOP principles. [1] [2]Post_Data,Primary_Term) for use within the new controller classes, and moved their locations/namespaces accordingly. [1] [2]Codebase organization and clean-up:
Abstract_Controllerand traits to a newAbstractsandTraitsdirectory, and updating references throughout the codebase. [1] [2] [3]CSS and visual improvements:
Changelog and documentation:
CHANGELOG.mdto reflect the new OOP/MVC approach for dynamic blocks and other recent changes.These changes set the stage for more scalable and maintainable block development going forward, and provide a clear separation of concerns between data handling and presentation.
QA
Links to relevant issues