-
Notifications
You must be signed in to change notification settings - Fork 3
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
SHS-5775: Implementation: Social Media Footer Block #1619
base: develop
Are you sure you want to change the base?
Conversation
@cienvaras - I've added the template suggestion so this is now ready for your review. |
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.
@codechefmarc Works as expected, thanks!
@ahughes3 Ready for you to test.
…-5775--social-footer
👀 I did a cursory review of this and looks good. I just want to take a closer look at the database update and .module hook. @codechefmarc There's a conflict in the |
@joegl Merge conflicts fixed! |
@codechefmarc I'm curious why you went with a custom block built via the UI and a new paragraph type, instead of creating a custom block plugin programmatically. Because of the large hook to format the icons based on the link URL's, it feels like a block plugin could have worked really well here and kept everything organized in a single block instance with a configuration form, instead of piecing together hooks and field/paragraph configuration. I think this could all comfortably fit into a single block plugin and would prefer that route if possible. |
@joegl - This was discussed internally before we started this ticket and we thought it was best to create a block via the UI because of the repeating mulit-value field with the paragraph, maintainability, and the ability to more easily add fields to this block if needed via the UI. I certainly can see it both ways - there are pros and cons to building this way vs a custom block plugin. |
@joegl - Reverted permissions to 11.3.1-release version. Ready for a retest. Thanks! |
Ok, that makes sense. I think the use of a new paragraph type to capture a multi-value field for a block is a bit out of scope for the intended use of paragraphs. A basic URL field comes with a title and URL which could suffice for the label and URL being captured here. A single block plugin also would probably be easier to maintain than all the configuration and module hooks here. We can still move forward with this implementation and I understand the reasons you chose this route. But I do want to push again to use a custom block plugin instead if possible. |
@joegl - I'm going to create a new PR to address this and create a custom block instead of one via the UI. For the moment, I'll keep this PR open, but will mark it as don't merge and once we're happy with the other PR, we can close this one. |
Summary
Adds config and update hook to create new social media footer block.
Need Review By (Date)
2024-09-09
Urgency
low
Steps to Test
Grid layout:
List layout:
PR Checklist