Feature: rebuild support for custom notices #9
Conversation
| $notice = (new AdminNotice('test_id', function () { | ||
| return '<div>It is Stellar at StellarWP</div>'; | ||
| })) | ||
| ->custom(); | ||
| $expectedNoticeWithAttributes = "<div data-stellarwp-namespace-notice-id='test_id' data-stellarwp-namespace-location='below_header'>It is Stellar at StellarWP</div>"; | ||
|
|
||
| $renderAdminNotice = new RenderAdminNotice('namespace'); | ||
|
|
||
| $tagProcessorMock = $this->createMock(TagProcessorMock::class); | ||
|
|
||
| $tagMatcher = $this->exactly(2); | ||
| $tagProcessorMock | ||
| ->expects($tagMatcher) | ||
| ->method('set_attribute') | ||
| ->willReturnCallback(function (string $key, string $value) use ($tagMatcher) { | ||
| switch ($tagMatcher->getInvocationCount()) { | ||
| case 1: | ||
| $this->assertEquals('data-stellarwp-namespace-notice-id', $key); | ||
| break; | ||
| case 2: | ||
| $this->assertEquals('data-stellarwp-namespace-location', $key); | ||
| $this->assertEquals('below_header', $value); | ||
| break; | ||
| } | ||
| }); | ||
|
|
||
| $tagProcessorMock | ||
| ->expects($this->once()) | ||
| ->method('next_tag'); | ||
|
|
||
| $tagProcessorMock | ||
| ->expects($this->once()) | ||
| ->method('__toString') | ||
| ->willReturn($expectedNoticeWithAttributes); | ||
|
|
||
| $this->setClassMock('WP_HTML_Tag_Processor', $tagProcessorMock); | ||
|
|
||
| $this->assertEquals($expectedNoticeWithAttributes, $renderAdminNotice($notice)); |
There was a problem hiding this comment.
This could be shortened and require less logic if you were to use snapshot testing (and require less rework when we change attributes):
$notice = (new AdminNotice('test_id', function () {
return '<div>It is Stellar at StellarWP</div>';
}))
->custom();
$renderAdminNotice = new RenderAdminNotice('namespace');
$this->assertMatchesStringSnapshot( $renderAdminNotice( $notice ) );This would necessitate the following composer package: lucatume/codeception-snapshot-assertions and Asserts added to the unit.suite.yml file (here's an example)
pup uses snapshot testing all over the place to capture CLI output, but it is great for arbitrary string comparisons that we want to persist over time. Here's an example in pup.
There was a problem hiding this comment.
I'd like your help with this since I haven't used this before. For now, I'll leave this test as it is working and does the job, and will look forward to refactoring it later with you! 😄
|
Ready for re-review, @borkweb! Thank you! |
JasonTheAdams
changed the title
Resolves #4
Description
This PR is a fairly significant rebuild of how custom notices work. A fundamentally different approach was taken than before, which used the confusing concept of "wrappers". It also had the problem that, when trying to position a custom notice, you still ended up fighting with the standard notice styling.
As described in the related Issue, a new approach was taken for custom notices. Namely, the
$notice->custom()method. All "wrapper" methods and such are dropped. Standard notices just do their thing, and custom notices use thecustom()method. This completely changes how custom notices are rendered and handled.When a custom notice is rendered, it applies the notice attribute and the notice location attribute to the outer custom notice HTML. The ID is used to identify the notice, and the location is used to move the notice to where it should go. Presently, the standard locations are supported, and the JS was updated to handle this — as WP is no longer handling it, due to styling issues.
Next, I added a new
NoticeElementPropertiesDTO. This object's sole purpose is to make adding the rather crazy attribute names easy. The notice and properties object is now passed to the render callback, for easy use.Finally, applying
$elementProperties->customCloserAttributes()attributes to any node will cause that node to close the notification. Note that the closer doesn't actually have to be inside the notice — and can be anywhere.For more information on these new methods, please refer to the README changes in the files changes.
Next Ideas
I had #6 in mind big time while building this. I believe the new DTO will really help with custom templates, as they'll effectively work closely to how the render callback works in this PR.
It also occurred to me, while making this, that it would be handy to have a way of enqueueing CSS or JS as part of a notice. That way the files are only actually loaded on the page if the notice passes all the visibility conditions.
Release Thoughts
This is technically backwards-breaking, but only for custom notices. I'm thinking this may merit a 2.0 release, but given how new and barely used this is, I'm tempted to do it as a minor release.