[add-resource] New Plugin

[aricooperdavis]

I would like to add my new plugin to the Grav Repository. Here are the project details: aricooperdavis/grav-plugin-custom-banner

[pamtbaau]

@aricooperdavis, I've taken a look at your proposed plugin...

It seems function onOutputGenerated() is quite error prone when the user doesn't provide all fields or provides fields without proper values in custom-banner.yaml. An Exception will then be thrown to the user, who might not understand the default error message, let alone know how to fix it.

Missing values: One way to prevent errors for missing values is to provide default values for each field in blueprint.yaml and collect these defaults using Config::getDefaults(). The defaults and the values in custom-banner.yaml can then be merged.

$config = $this->config();                                             <-- values from 'custom-banner.yaml'
$defaults = $this->config->getDefaults()['plugins']['custom-banner'];  <-- values from 'blueprint.yaml'
$mergedConfig = array_merge($config, $defaults);

Invalid exclude-pages: You might consider testing if the value of exclude-pages is either a string, or an array.

Default value for exclude-pages The value for button-url might be added to exclude-pages at runtime. It seems logical that the banner is not visible on the page for 'Read more'.

[aricooperdavis]

Thanks for your tips pamtbaau, that's really helpful!

Missing values: I've implemented a merge with the defaults as you've suggested. As such I have made the defaults more complex to reduce the likelihood of collisions (for example if the exclude-pages is left blank the merge will use the default value of ['/route-to-page/you-want-to/exclude'] which is very unlikely to be an in-use route). We also now validate the merged config against the plugin blueprint to catch mistakes like 'false' being evaluated as true (which will show the error message, but it's a sensible message and I can't think of a better way to feed that back to the user).

Invalid exclude-pages: The config value for exclude-pages is now cast to an array.

Default value for exclude-pages: I can see why this could be sensible but I consider that to be unintuitive behaviour - if you've got an exclude-pages field in the config then I expect only the pages in there to be excluded. I guess I could add another boolean config option to exlude the linked-to page but this feels over-complicated when users could just select the page they're linking to. There's also no guarentee that users are linking to a page on the site rather than an external link.

Thank you again, do you have any other changes you'd like to see?

[pamtbaau]

Great! Do I have any other changes I would like to see? Sure...

• While testing the banner it annoyed me I couldn't dismiss it. It seems it's already on your todo list...
• Option to have banner on top or at bottom?
• You might be a bit more specific in the README about the term 'banner'. This is a very generic term. My first thought was a kind of inline ad at the top of the page.
  • Maybe you could add a few example use-cases when to use it and when not?
  • Maybe add a screenshot?

[aricooperdavis]

Thanks again for the excellent feedback. I have made some of those changes. I think we should probably do further feature-request style discussion on the repo itself to keep this issue clean?

[pamtbaau]

Yes, we should move over to your repo...

By the way, nice plugin! Simple, clean, effective...

[aricooperdavis]

Anything I need to do to facilitate the plugin being added @mahagr?

[rhukster]

I've added this to GPM so should be available soon.