Skip to content
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

Feature: Widgets and Snippets #382

Merged
merged 89 commits into from Apr 22, 2019

Conversation

@bobdenotter
Copy link
Member

commented Apr 3, 2019

Fixes #111. Fixes #276. Fixes #135.

@bobdenotter bobdenotter force-pushed the feature/widgets-and-snippets branch from c1e52d9 to 9dd80d7 Apr 6, 2019

@bobdenotter bobdenotter changed the title [WIP] Feature: Widgets and Snippets Feature: Widgets and Snippets Apr 8, 2019

@bobdenotter bobdenotter added this to the Bolt 4 beta 1 milestone Apr 10, 2019

@bobdenotter bobdenotter requested a review from JarJak Apr 10, 2019

src/Controller/TwigAwareController.php Outdated Show resolved Hide resolved
src/EventSubscriber/ZoneSubscriber.php Outdated Show resolved Hide resolved
src/EventSubscriber/ZoneSubscriber.php Outdated Show resolved Hide resolved
src/Widget/NewsWidget.php Show resolved Hide resolved
src/Widget/BoltHeaderWidget.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Widget/BoltHeaderWidget.php Outdated Show resolved Hide resolved
tests/php/SnippetsTest.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippet/Zone.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Snippets.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Controller/Frontend/FrontendZone.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widget/BoltHeaderWidget.php Outdated Show resolved Hide resolved
src/Widgets.php Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Snippet/Injector.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
src/Widgets.php Outdated Show resolved Hide resolved
/** @var Response */
protected $response;
public function setName(string $name): self

This comment has been minimized.

Copy link
@JarJak

JarJak Apr 14, 2019

Member

Not when there will be an interface. Values can be set during construction.

@bobdenotter

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Not when there will be an interface. Values can be set during construction.

The can, but they don't have to be.. This should be perfectly fine:

        $metaTagSnippet = (new SnippetWidget())
            ->setSnippet('<meta name="generator" content="Bolt">')
            ->setName('Meta Generator tag snippet')
            ->setTarget(Target::END_OF_HEAD)
            ->setZone(Zone::FRONTEND);

I think it's a perfect usecase for people to create a snippet, and then set different properties based on their program logic.

@bobdenotter

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

If that won't work too, lets keep it as is.

Nope, doesn't work.

@JarJak

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Something that is not going to be changed during program execution should not be set through setters. So it should look like:

$metaTagSnippet = new SnippetWidget(
            '<meta name="generator" content="Bolt">'
            'Meta Generator tag snippet'
            Target::END_OF_HEAD
            Zone::FRONTEND
);
src/Widget/RequestAware.php Outdated Show resolved Hide resolved
src/Twig/WidgetExtension.php Outdated Show resolved Hide resolved
src/Widget/RequestAware.php Show resolved Hide resolved
src/Widget/TwigAware.php Show resolved Hide resolved
@bobdenotter

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Something that is not going to be changed should not be set through setters.

But it can change. we should not disallow that.

So it should look like:

I'm not certain I like that better, but i've updated the code to allow for this. The Setters are staying, though.

@JarJak

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Allowing both ways is even worse. Anyway user should be able to implement a Widget without using BaseWidget as abstract base class, but by implementing WidgetInterface. By removing setters we are not disallowing anything because using BaseWidget is optional.

@bobdenotter

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Allowing both ways is even worse.

I disagree. People want to use things in different ways.

Anyway user should be able to implement a Widget without using BaseWidget as abstract base class, but by implementing WidgetInterface.

This I agree with. Will work on making it into an interface.

By removing setters we are not disallowing anything because using BaseWidget is optional.

Ok, so Basewidget will have the setters, but it's optional, because it'll not be in the WidgetInterface. A developer can then choose to forego Basewidget, but implement WidgetInterface instead.

Thinking about that, shouldn't the interfaces RequestAware and TwigAware be Traits instead?

src/Widget/WidgetInterface.php Show resolved Hide resolved
src/Snippet/HtmlInjector.php Outdated Show resolved Hide resolved
src/Widget/BaseWidget.php Outdated Show resolved Hide resolved
src/Widget/BaseWidget.php Outdated Show resolved Hide resolved
src/Widget/BaseWidget.php Outdated Show resolved Hide resolved
src/Widget/WidgetInterface.php Outdated Show resolved Hide resolved
src/Widget/WidgetInterface.php Outdated Show resolved Hide resolved

bobdenotter added some commits Apr 21, 2019

src/Widget/TwigAware.php Outdated Show resolved Hide resolved

bobdenotter added some commits Apr 21, 2019

src/Widget/BaseWidget.php Outdated Show resolved Hide resolved

bobdenotter and others added some commits Apr 21, 2019

Jarek Jakubowski
Jarek Jakubowski
Change BaseWidget description
Co-Authored-By: JarJak <egger1991@gmail.com>
Jarek Jakubowski
tests/php/WidgetsTest.php Outdated Show resolved Hide resolved

Jarek Jakubowski and others added some commits Apr 21, 2019

Jarek Jakubowski
Jarek Jakubowski
Jarek Jakubowski
Jarek Jakubowski
Widgets changes (#411)
Widgets changes
@JarJak

JarJak approved these changes Apr 22, 2019

@JarJak JarJak merged commit 64174e9 into master Apr 22, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@JarJak JarJak deleted the feature/widgets-and-snippets branch Apr 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.