Port over paint kit hero widget #170
Conversation
|
Changing the array syntax over to 5.2 compatible right now |
|
@jonathanbardo I started a review but stopped to discuss a broader point first. I assume the idea was that we might add more widgets later. But I'm wondering if there's a case to be made that we might be over-engineering it. TBH this feels like a shitload of code to maintain for just a simple widget with 4 fields. Could it be simpler to just cross that road when we come to it? |
|
@fjarrett I could get rid of the base class if you feel more comfortable. It's basically the same pattern as contact widget so I don't know if it adds that much more burden on our shoulder since we maintain the other one anyway. In any case I can bring it down to 1 php class and 1 js file. |
|
@jonathanbardo I've been thinking a little about CW too, in light of godaddy-wordpress/contact-widgets#22, which required significant changes to the base class to the point where we had to table it until the next major release. Rather than the base class being an aid to speed things up, instead, we had to manhandle the HOO widget to be compatible with the pattern. |
|
@fjarrett I've consolidated the necessary logic in class-hero.php and got rid of the base class. I think you can give this another look. |
frankiejarrett
requested review from
EvanHerman
and removed request for
frankiejarrett
|
@EvanHerman and @jonathanbardo please have a look at this and test. My goal was to reduce the widget code to its simplest and purest form. |
inc/hero-text-widget.php
Outdated
|
|
||
| parent::__construct( 'primer-hero-text', esc_html__( 'Hero Text', 'primer' ), $widget_options ); | ||
|
|
||
| add_action( 'admin_enqueue_scripts', array( $this, 'enqueue_scripts' ) ); |
There was a problem hiding this comment.
@fjarrett Will this only enqueue the script if we are on the widget page or in the customizer?
There was a problem hiding this comment.
@fjarrett I see that you are checking for specific hook in the method itself. That was the reason I only added the action when the form was printed and in the footer only. That way we know the widget is being printed on the page.
There was a problem hiding this comment.
@jonathanbardo But doesn't that require a page reload? What happens if you drag the widget into a sidebar for the first time on widgets.php?
There was a problem hiding this comment.
@fjarrett It should work as expected even if the widget is not in any sidebars because the form is printed anyways.
inc/hero-text-widget.php
Outdated
| ( function ( $ ) { | ||
|
|
||
| // This let us know that we appended a new widget to reset sortables. | ||
| // Let our script know a widget has been added to make its URL search input work. |
There was a problem hiding this comment.
@jonathanbardo I updated the comment here to better clarify why the inline script exists.
|
@EvanHerman please test this and merge if you approve |
EvanHerman
left a comment
EvanHerman
left a comment
There was a problem hiding this comment.
@fjarrett Works well for me. Merging in.
@fjarrett Please review.