-
Notifications
You must be signed in to change notification settings - Fork 8
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
Make banner block type display configurable. #109
Make banner block type display configurable. #109
Conversation
j4-m
commented
Jan 25, 2021
- Adds a block form which allows admins to choose the alter banner types they want the block to display
- Use the configuration to filter the queried alter banners before render.
* Adds a block form which allows admins to choose the alter banner types they want the block to display * Use the configuration to filter the queried alter banners before render.
Thanks @Iam-Jam |
Moving to WIP as we'll need to update the tests to cover the new configuration. |
Fixes indentation.
Thanks for your work so far @Iam-Jam.
|
No worries @andybroomfield. As of the latest commits, there shouldn't be any errors when saving the configuration form and hopefully the schema issue has been resolve too... just need add some test coverage now. |
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.
Thanks for your efforts on this @Iam-Jam.
I'm still seeing the following error on the block configuration page when first installing the module.
Error message
Warning: Invalid argument supplied for foreach() in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 100 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).
To replicate, do a fresh install and enable localgov_alert_banner. Then go to the block configuration screen without having placed a block.
The error will disapear once I save the block for the first time.
Additonal amends:
- Can we change the label Display types to Banner types to match the admin view.
- Would it be possible to have the negate this condition option added
I can confirm the same error. |
'#options' => $config_options, | ||
'#title' => $this->t('Display types'), | ||
'#description' => $this->t('If no types are selected all will be displayed.'), | ||
'#default_value' => !empty($config['include_types']) ? $config['include_types'] : '', |
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.
@finnlewis @Iam-Jam I think this might be what is causing the error.
I think default value should be an array (even if it is null).
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.
Yep that's it @andybroomfield. I had actually spotted this before but forgot to fix it. Shame this hasn't been picked up in tests.
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.
Just to confirm that changing
'#default_value' => !empty($config['include_types']) ? $config['include_types'] : '',
to
'#default_value' => !empty($config['include_types']) ? $config['include_types'] : [],
resolves this issue. If we can get this change added we can get this in a new release.
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.
Ditto. @j4-m could you commit the change to your forked branch?
Yes - I got the same place following the debugger and the code changes :) |
Hi @j4-m Thanks for your efforts so far. I'm stlll getting the error, did we get anywhere with this? |
@andybroomfield @finnlewis the amendment to the default value has just gone in. |
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.
Error now fixed so clear to merge @j4-m