Feedback - Ideas - Feature Requests

[stpaultim]

General Feedback on this Module

This is what the UI looks like right now.

[yorkshire-pudding]

One thought I had is allowing this to be defined for other roles also rather than just authenticated and unauthenticated. You might then have n fields where users can add a line, select the role and define the content.

[docwilmot]

As far as code goes:

• In conditional_content_block_block_view() move these lines https://github.com/backdrop-contrib/conditional_content_block/blob/146477f945503ffc99d48e7b685fe8bb8e61ea71/conditional_content_block.module#L60-L66 to after the case 'conditional': line, since they only need to run if a user is viewing this block.
• You've got Implements hook_block_configure() twice
• In block view, $markup needs sanitizing, probably with filter_xss_admin() since it seems its intended to be html, since its coming from a text_format widget.
• This line https://github.com/backdrop-contrib/conditional_content_block/blob/146477f945503ffc99d48e7b685fe8bb8e61ea71/conditional_content_block.module#L42 uses an $edit variable. I'm assuming should have been $settings.
• The field $form['ccb_user_message'] doesnt define text format and upload etc, is that intentional?
• You would use a config folder and some defaults, and need a hook_config_info() since you're creating a config setting. But layout takes care of saving block data, so you shouldnt need a config setting. I see you have a separate issue re saving to config, so perhaps youre aware of that.
• So final thing, if youre keeping config, is in functions where you call config_get() multiple times, its better to call $config = config('conditional_content_block.settings') once instead.

[stpaultim]

One thought I had is allowing this to be defined for other roles also rather than just authenticated and unauthenticated. You might then have n fields where users can add a line, select the role and define the content.

Thanks. I did have this in mind as one possible route forward. I appreciate the feedback.

@docwilmot - thanks for all the suggestions. They look very helpful, I need to dive in and look them closer. We are getting into areas in which I'm very much a beginner and rely a lot on hacking existing code. At this point, my main goal was to get SOMETHING that worked - a simple version. Now, I'm trying to figure out next steps.

[stpaultim]

@docwilmot - Implemented all your suggestions. All good and helpful in terms of me understanding what I'm doing. Started out hacking existing code without fully understanding it, as of now I have a better understanding of what most of the code is doing.

Big thanks.

[docwilmot]

Glad to help. One small thing, filter_xss_admin only takes strings, but you're passing it an array.

[stpaultim]

@docwilmot - Is this better?

      $block_first_text = $settings['ccb_first_message'];
      $block_second_text = $settings['ccb_second_message'];
      $block_first_message = filter_xss_admin($block_first_text['value']);
      $block_second_message = filter_xss_admin($block_second_text['value']);

[docwilmot]

Yep that looks good