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

SHS-6009: Remove Visibility and Region fields from the Social Media footer block for Site Editors #1715

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

codechefmarc
Copy link
Collaborator

@codechefmarc codechefmarc commented Jan 13, 2025

Summary

When editing the social media footer block, site managers will no longer see the "Visibility" and "Region" fields. Developer roles will still see these fields.

Need Review By (Date)

2025-01-15

Urgency

low

Steps to Test

  1. Login to the site as a Developer
  2. Add a social media block to the system (typically in the footer)
  3. Verify that the Visibility and Region fields are present

Screenshot 2025-01-13 at 12 38 52 PM

  1. Add in a link and then save the block
  2. Masquerade as a site manager (Lindsay is usually our test user for this case)

Screenshot 2025-01-13 at 12 42 26 PM

  1. Go the home page
  2. Hover over the social media block and click the pencil icon, then clock "Edit Social Media block"

Screenshot 2025-01-13 at 12 40 20 PM

  1. Verify that as a site manager role, the Visibility and Region fields are no longer visible

Screenshot 2025-01-13 at 12 41 29 PM

Review Tasks

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Are PHP functions and variables in snake_case and not camelCase?
  • Does Drupal code follow Drupal Coding Standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided?

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

@codechefmarc codechefmarc self-assigned this Jan 13, 2025
@codechefmarc codechefmarc changed the base branch from develop to 11.6.3-release January 13, 2025 20:25
@ahughes3 ahughes3 temporarily deployed to Tugboat January 13, 2025 20:28 Destroyed
@codechefmarc codechefmarc marked this pull request as ready for review January 13, 2025 22:04
Copy link
Collaborator

@callinmullaney callinmullaney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codechefmarc Nice work! Confirming that I no longer see these visibility regions for non-administrator roles.
Screenshot 2025-01-15 at 8 47 57 AM

@callinmullaney
Copy link
Collaborator

@ahughes3 Functional tests pass and the code looks good to me. Kicking this over to you for confirmation.

@ahughes3 ahughes3 temporarily deployed to Tugboat January 15, 2025 16:58 Destroyed
Base automatically changed from 11.6.3-release to develop January 22, 2025 16:25
@ahughes3 ahughes3 temporarily deployed to Tugboat January 23, 2025 19:04 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat January 24, 2025 01:12 Destroyed
Copy link
Collaborator

@ahughes3 ahughes3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to go!

@ahughes3 ahughes3 assigned joegl and unassigned ahughes3 Jan 25, 2025
@ahughes3 ahughes3 requested a review from joegl January 25, 2025 01:24
@joegl joegl changed the base branch from develop to 11.6.4-release January 27, 2025 21:45
Copy link
Contributor

@joegl joegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codechefmarc Can this be done in the block plugin itself without using a hook?

public function blockForm($form, FormStateInterface $form_state): array {

@codechefmarc
Copy link
Collaborator Author

@codechefmarc Can this be done in the block plugin itself without using a hook?

public function blockForm($form, FormStateInterface $form_state): array {

@joegl - I looked into this and there doesn't appear to be a way to do it via the block plugin we have already. This is because the $form array in the plugin does not have those visibility or region keys like it does on the form alter. There may be a path if we overrode docroot/core/modules/block/src/BlockForm.php (which is where the visibility and region settings ultimately come from) via a ServiceProvider class or something akin to that. However, I believe that would be much more heavy handed than what we came up with the form alter hook. If you want us to pursue this route, let me know, I'm happy to dig deeper.

@joegl
Copy link
Contributor

joegl commented Jan 28, 2025

@codechefmarc Can this be done in the block plugin itself without using a hook?

public function blockForm($form, FormStateInterface $form_state): array {

@joegl - I looked into this and there doesn't appear to be a way to do it via the block plugin we have already. This is because the $form array in the plugin does not have those visibility or region keys like it does on the form alter. There may be a path if we overrode docroot/core/modules/block/src/BlockForm.php (which is where the visibility and region settings ultimately come from) via a ServiceProvider class or something akin to that. However, I believe that would be much more heavy handed than what we came up with the form alter hook. If you want us to pursue this route, let me know, I'm happy to dig deeper.

Nope, that works for me 👍 I'd always prefer using OOP instead of a hook in a .module file, but that's not always possible.

@joegl joegl merged commit d8e9fc1 into 11.6.4-release Jan 28, 2025
18 checks passed
@joegl joegl deleted the SHS-6009--social-media-field-visibility branch January 28, 2025 17:10
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.

4 participants