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

Rework the front end preview #989

Merged
merged 30 commits into from
Jan 6, 2020
Merged

Conversation

richardhj
Copy link
Member

@richardhj richardhj commented Nov 15, 2019

This PR injects the back end preview bar and replaces the preview frameset.

How this works:

The preview bar is injected into the DOM on kernel.response event.
When the user wants to view the page as a member, an Ajax request is triggered in which the preview authentication will be handled. The Ajax endpoint logs in the user, returns an HTTP OK and finally, a page reload is done.

TODO:

Impressions:
Screen Shot 2019-11-24 at 16 09 40


Outlook:
I already worked on an enhanced version of the preview. Once this PR is approved and merged, I add a framework for 3rd parties to add content to the preview toolbar.
So-called "preview providers" (class must implementing an interface and tag the service with contao.preview_provider) can add arbitrary content to the toolbar.
In the end, the great advantage of the preview is the back end scope in the front end.
The toolbar should be either horizontally (as we know it) or left-aligned when the content requires it.

Use case: theme switch or changing CSS variables on runtime.

Screen Shot 2019-11-24 at 20 52 19

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!! Really looking forward to this!

core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
@leofeyer leofeyer added this to the 4.9 milestone Nov 15, 2019
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Can we maybe add an image to see what the toolbar will look like?

Any particular reason why the user list is loaded with ajax?

@richardhj richardhj changed the title Injected back end preview toolbar [WIP] Injected back end preview toolbar Nov 22, 2019
@richardhj richardhj changed the title [WIP] Injected back end preview toolbar [WIP] Rework front end preview Nov 24, 2019
@richardhj richardhj changed the title [WIP] Rework front end preview [RFC] Rework front end preview Nov 25, 2019
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks excellent to me. Imho, this can be extended with unit tests and merged as is!

Polishing CSS and adding more toolbar functionality such as JS can still be done afterwards. At least we got rid of the iframe now and the preview bar is shown consistently everywhere :) Great work!

core-bundle/src/Controller/BackendPreviewController.php Outdated Show resolved Hide resolved
core-bundle/src/Controller/BackendPreviewController.php Outdated Show resolved Hide resolved
core-bundle/src/Controller/BackendPreviewController.php Outdated Show resolved Hide resolved
core-bundle/src/Controller/BackendPreviewController.php Outdated Show resolved Hide resolved
core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
@richardhj richardhj changed the title [RFC] Rework front end preview Rework front end preview Nov 30, 2019
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the Mumble call yesterday, $user->amg is the correct implementation, but it should be in the BackendUser class instead.

@leofeyer
Copy link
Member

$user->amg is the correct implementation, but it should be in the BackendUser class instead.

#1120 has been merged, so this can be added now. 👍

@leofeyer leofeyer changed the title Rework front end preview Rework the front end preview Dec 23, 2019
# Conflicts:
#	calendar-bundle/tests/DependencyInjection/ContaoCalendarExtensionTest.php
#	calendar-bundle/tests/EventListener/PreviewUrlConverterListenerTest.php
#	core-bundle/src/Resources/contao/library/Contao/User.php
#	news-bundle/tests/DependencyInjection/ContaoNewsExtensionTest.php
#	news-bundle/tests/EventListener/PreviewUrlConverterListenerTest.php
@richardhj
Copy link
Member Author

I addressed the changes and merged against master to resolve conflicts.

aschempp
aschempp previously approved these changes Jan 6, 2020
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good to me! Unfortunately, the coverage seems to decrease quite a lot, are we missing tests on new classes?

Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks very good to me. 👍 We cannot merge it with the decreasing test coverage though.

core-bundle/src/EventListener/PreviewToolbarListener.php Outdated Show resolved Hide resolved
Toflar
Toflar previously approved these changes Jan 6, 2020
@leofeyer leofeyer merged commit c87c6a2 into contao:master Jan 6, 2020
@leofeyer
Copy link
Member

leofeyer commented Jan 6, 2020

Thank you @richardhj. 🎉

@Toflar
Copy link
Member

Toflar commented Jan 6, 2020

🎉 thanks, Richard! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants