Problem/Motivation

The rendered diff should look as much as possible as the original article.
Thus if the theme fits, it should use the frontend theme.

In contrast, we currently use the backend theme.

Proposed resolution

Offer a setting to declare if the rendered diff should use the frontend theme.
This should only apply to the rendered entity diff.

By default, we recommend frontend.
Usually, this is done by setting the route as admin route = backend theme, but dunno if we can do this dynamically...

User interface changes

Let the rendered diff appear in frontend theme.

CommentFileSizeAuthor
#74 interdiff-2799597-70-74.txt605 bytestoncic
#74 select_theme_for_diff-2799597-74.patch6.8 KBtoncic
#70 interdiff-2799597-67-70.txt1.96 KBtoncic
#70 select_theme_for_diff-2799597-70.patch7.39 KBtoncic
#67 interdiff-2799597-65-67.txt1.88 KBtoncic
#67 select_theme_for_diff-2799597-67.patch7.62 KBtoncic
#65 interdiff-2799597-63-65.txt567 bytestoncic
#65 select_theme_for_diff-2799597-65.patch7.56 KBtoncic
#63 interdiff-2799597-61-63.txt1.28 KBtoncic
#63 select_theme_for_diff-2799597-63.patch8.12 KBtoncic
#61 interdiff-2799597-56-61.txt2.94 KBtoncic
#61 select_theme_for_diff-2799597-61.patch8.09 KBtoncic
#56 interdiff-2799597-53-56.txt4.89 KBjohnchque
#56 select_theme_for_diff-2799597-56.patch7.92 KBjohnchque
#53 select_theme_for_diff-2799597-53.patch8.71 KBtoncic
#50 interdiff-2799597-48-50.patch1.63 KBtoncic
#50 select_theme_for_diff-2799597-50.patch8.71 KBtoncic
#48 interdiff-2799597-45-48.txt1.98 KBtoncic
#48 select_theme_for_diff-2799597-48.patch8.72 KBtoncic
#45 interdiff-2799597-43-45.txt2.28 KBtoncic
#45 select_theme_for_diff-2799597-45.patch8.48 KBtoncic
#43 interdiff-2799597-41-43.txt719 bytestoncic
#43 select_theme_for_diff-2799597-43.patch7.97 KBtoncic
#41 interdiff-2799597-39-41.txt481 bytestoncic
#41 select_theme_for_diff-2799597-41.patch7.87 KBtoncic
#39 interdiff-2799597-34-39.txt465 bytestoncic
#39 select_theme_for_diff-2799597-39.patch7.87 KBtoncic
#36 select_theme_for_diff-2799597-36.patch7.86 KBtoncic
#34 select_theme_for_diff-2799597-34_without_test.patch5.16 KBtoncic
#32 select_theme_for_diff-2799597-31_without_test.patch5.16 KBtoncic
#30 interdiff-2799597-28-30.txt2.01 KBtoncic
#30 select_theme_for_diff-2799597-30.patch7.86 KBtoncic
#28 interdiff-2799597-25-28.txt950 bytestoncic
#28 select_theme_for_diff-2799597-28.patch7.96 KBtoncic
#25 interdiff-2799597-22-25.txt6.2 KBtoncic
#25 select_theme_for_diff-2799597-25.patch7.96 KBtoncic
#22 interdiff-2799597-18-22.txt1.25 KBtoncic
#22 select_theme_for_diff-2799597-22.patch5.17 KBtoncic
#21 interdiff-2799597-15-18.txt1.83 KBtoncic
#18 visual_inline_layout.png68.78 KBtoncic
#18 general_settings.png73.23 KBtoncic
#18 interdiff-2799597-15-18.patch1.83 KBtoncic
#18 select_theme_for_diff-2799597-18.patch5.01 KBtoncic
#15 interdiff-2799597-12-15.txt5.3 KBtoncic
#15 select_theme_for_diff-2799597-15.patch4.6 KBtoncic
#12 interdiff-2799597-8-12.txt1.62 KBtoncic
#12 select_theme_for_diff-2799597-12.patch4.37 KBtoncic
#8 interdiff-2799597-6-8.txt1.14 KBtoncic
#8 select_theme_for_diff-2799597-8.patch4.22 KBtoncic
#6 interdiff-2799597-3-6.txt1.23 KBtoncic
#6 select_theme_for_diff-2799597-6.patch3.66 KBtoncic
#4 theme.png47.07 KBtoncic
#3 select_theme_for_diff-2799597-3.patch3.22 KBtoncic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker created an issue. See original summary.

Berdir’s picture

only for one diff format means it doesn't only depend on the route, otherwise you could do a route alter and handle it like node (which has a setting if node edit pages should be use frontend or backend theme. See \Drupal\node\EventSubscriber\NodeAdminRouteSubscriber::alterRoutes(). We could do something similar if it would be always there on a given route or not. Could be an option like node or just apply it to specific routes.

If based on the filter query argument, then it needs a custom theme negotiator. Would not be called before \Drupal\user\Theme\AdminNegotiator and set the default if the option is set.

toncic’s picture

Assigned: Unassigned » toncic
Status: Active » Needs review
FileSize
3.22 KB

Trying to added theme for visual_inline layout.

toncic’s picture

FileSize
47.07 KB

Providing screenshot.

Berdir’s picture

Status: Needs review » Needs work
+++ b/src/FrontEndNegotiator.php
@@ -0,0 +1,35 @@
+  public function applies(RouteMatchInterface $route_match) {
+   if ($route_match->getParameter('filter') === 'visual_inline') {
+     if ($_REQUEST['theme'] === 'front_end') {
+       return TRUE;
+     }
+   }

never use $_REQUEST, use the request object. You also dropped the route name check now that we did together, you still need that, we only want to check on that page.

And since front_end will be the default (or maybe the default will even be configurable), you also need to return TRUE if there is no theme given at all.

toncic’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
1.23 KB

Implemented comm #5.

johnchque’s picture

Status: Needs review » Needs work

I like it really much, but:

+++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
@@ -154,6 +153,37 @@ class VisualInlineDiffLayout extends DiffLayoutBase {
+    $build['theme'] = [
+      '#type' => 'operations',
+      '#links' => $links,
+    ];

here we need a prefix and suffix as the other dropdown buttons.
Something like:

    $build['theme'] = [
      '#type' => 'item',
      '#title' => $this->t('Theme'),
      '#weigth' => 2,
      '#prefix' => '<div class="diff-layout">',
      '#suffix' => '</div>',
    ];
    $build['theme']['filter'] = [
      '#type' => 'operations',
      '#links' => $links,
      '#prefix' => '<div class="diff-filter">',
      '#suffix' => '</div>',
    ];

and we need to use code like:

$filter = $links[$active_view_mode];
    unset($options[$active_view_mode]);
    array_unshift($options, $filter); 

to set the current used theme as the default in the button.

toncic’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
1.14 KB

Applying comm #7.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
@@ -154,7 +153,52 @@ class VisualInlineDiffLayout extends DiffLayoutBase {
+    $theme_keys = array_keys($links);
+    $active_theme = $this->requestStack->getCurrentRequest()->query->get('theme') ?: reset($theme_keys);
+    $theme = $links[$active_theme];
+    unset($links[$active_theme]);
+    array_unshift($links, $theme);

do the unshift before this:
$build['theme'] = [
'#type' => 'item',
'#title' => $this->t('Theme'),
'#weigth' => 1,
'#prefix' => '

',
'#suffix' => '

',

Also please add some spaces to identify the code easier.

johnchque’s picture

+++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
@@ -154,7 +153,52 @@ class VisualInlineDiffLayout extends DiffLayoutBase {
+      '#prefix' => '<div class"diff-filter">',

missing "="

miro_dietiker’s picture

Oh, i was really confused from reading the patch first.

I'm talking of a setting and what i meant with it is:
There are websites with themes where displaying in frontend theme will not properly work.
The end user = content creator should not get the selection option in that case and the system should not offer anything in frontend.
So the setting would be a global diff setting in configuration.
The setting should only be visible if this plugin is enabled.

Since it's a UI change, please always provide screenshots.
I'm not sure if the end user needs to switch back and forth between frontend and backend...
The preview for instance also doesn't allow this.

toncic’s picture

Fixing typo and coding standard.

toncic’s picture

Status: Needs work » Needs review
miro_dietiker’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

You still didn't move the selection to the admin settings.

And tests. :-)

toncic’s picture

Moved button into general settings and test coverage.

Status: Needs review » Needs work

The last submitted patch, 15: select_theme_for_diff-2799597-15.patch, failed testing.

johnchque’s picture

Works niiiiiice, but

+++ b/src/Form/GeneralSettingsForm.php
@@ -112,6 +112,17 @@ class GeneralSettingsForm extends ConfigFormBase {
+    $form['theme'] = [
+      '#type' => 'select',
+      '#title' => $this->t('Theme'),
+      '#options' => [
+        'front_end' => 'Front end',
+        'back_end' => 'Back end',
+      ],
+      '#description' => $this->t('You can choose which theme you want to use for visual_inline layout.'),
+      '#default_value' => $config->get('general_settings')['theme'],
+    ];
+

add a

'#states' => array(
        'visible' => array(
          ':input[name=THENAMEOFTHEVISUALINLINECHECK]' => array('checked' => TRUE),
        ),
      ), 

to this, so it can be visible just when visual inline is checked.
Also, I think we would need to add a line in the diff.settings.yml file that will be the default theme when installing the module.

toncic’s picture

I am not sure that I added default theme on the right way.
Providing screenshots.

The last submitted patch, 18: select_theme_for_diff-2799597-18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: interdiff-2799597-15-18.patch, failed testing.

toncic’s picture

FileSize
1.83 KB

Sorry for interdiff.

toncic’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
1.25 KB

Fixing test fails.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/Form/GeneralSettingsForm.php
@@ -137,6 +137,21 @@ class GeneralSettingsForm extends ConfigFormBase {
+      '#description' => $this->t('You can choose which theme you want to use for visual_inline layout.'),

I think this can be nicer if says something like: "Theme used for Visual inline layout when comparing revisions."

Berdir’s picture

  1. +++ b/diff.services.yml
    @@ -27,4 +27,10 @@ services:
    +  theme.negotiator.front_end_theme:
    +    class: Drupal\diff\FrontEndNegotiator
    +    arguments: ['@current_user', '@config.factory', '@entity.manager', '@router.admin_context']
    +    tags:
    +      - { name: theme_negotiator, priority: 0}
    

    Don't think that is a very good name. Maybe something like VisualDiffThemeNegotiator since it is specifically about that?

  2. +++ b/src/Form/GeneralSettingsForm.php
    @@ -137,6 +137,21 @@ class GeneralSettingsForm extends ConfigFormBase {
    +      '#options' => [
    +        'front_end' => 'Front end',
    +        'back_end' => 'Back end',
    +      ],
    

    since this is now a backend setting, not sure about what options we want to display here exactly. I guess this is ok and selecting a specific setting is not necessary, but the strings need to be translatable at least.

    We could also name them Standard and Admin, that's possibly a bit more common naming for admins.

  3. +++ b/src/FrontEndNegotiator.php
    @@ -0,0 +1,38 @@
    +    if ($route_match->getRouteName() === 'diff.revisions_diff') {
    +      if ('front_end' === \Drupal::config('diff.settings')->get('general_settings.theme')) {
    +        if ($route_match->getParameter('filter') === 'visual_inline' ) {
    +          return TRUE;
    +        }
    +      }
    

    you can move the parameter check into the first if, together with the rout name. The config should be later, as we don't want to load the config unless we have to.

    We should also inject the configFactory and then get the config from there (but do not get the config object in the constructor, only when you need it)

  4. +++ b/src/FrontEndNegotiator.php
    @@ -0,0 +1,38 @@
    +    return $this->configFactory->get('system.theme')->get('default');
    

    actually we have that already, just do it above like you do here.

The tests also only cover the setting and not the actual functionality?

toncic’s picture

Status: Needs work » Needs review
FileSize
7.96 KB
6.2 KB

Added test coverage for functionality and changes from #24.

Status: Needs review » Needs work

The last submitted patch, 25: select_theme_for_diff-2799597-25.patch, failed testing.

johnchque’s picture

+++ b/src/Tests/AdminFormsTest.php
@@ -33,12 +33,15 @@ class AdminFormsTest extends DiffTestBase {
+      'theme' => 'back_end',

Why back_end?

+++ b/diff.services.yml
@@ -27,4 +27,10 @@ services:
+  theme.negotiator.standard_theme:

As discussed, change this name.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.96 KB
950 bytes

Implement comm #27.

Status: Needs review » Needs work

The last submitted patch, 28: select_theme_for_diff-2799597-28.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.86 KB
2.01 KB

Patch rebased and small clean patch.

Status: Needs review » Needs work

The last submitted patch, 30: select_theme_for_diff-2799597-30.patch, failed testing.

toncic’s picture

Uploaded test without test.

Status: Needs review » Needs work

The last submitted patch, 32: select_theme_for_diff-2799597-31_without_test.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
5.16 KB

Trying one more time without test for functionality.

johnchque’s picture

Status: Needs review » Needs work
+++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
@@ -0,0 +1,81 @@
+  public function testNegotivator() {

"Negotiator" right?

toncic’s picture

Status: Needs work » Needs review
FileSize
7.86 KB

Added test for functionality.

Status: Needs review » Needs work

The last submitted patch, 36: select_theme_for_diff-2799597-36.patch, failed testing.

The last submitted patch, 36: select_theme_for_diff-2799597-36.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
465 bytes

Added @group annotation.

Status: Needs review » Needs work

The last submitted patch, 39: select_theme_for_diff-2799597-39.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
481 bytes

Changed @group.

Status: Needs review » Needs work

The last submitted patch, 41: select_theme_for_diff-2799597-41.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
719 bytes

This should works.

johnchque’s picture

Status: Needs review » Needs work
+++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
@@ -0,0 +1,86 @@
+    $route_match = $this->prophesize(RouteMatchInterface::class);
+    $route_match->getRouteName()->willReturn('diff.revisions_diff');
+    $route_match->getParameter('filter')->willReturn('visual_inline');
+    $container = $this->prophesize(ContainerInterface::class);
+    $config_factory = $this->prophesize(ConfigFactory::class);
+    $container->get('config.factory')->willReturn($config_factory);
+    $diff_config = $this->prophesize(ImmutableConfig::class);
+    $config_factory->get('diff.settings')->willReturn($diff_config);
+    $diff_config->get('general_settings.theme')->willReturn('standard');
+    \Drupal::setContainer($container->reveal());
+    $this->assertTrue($theme->applies($route_match->reveal()));

It might be worth to add some comment here, at least in the assertTrue.

toncic’s picture

Status: Needs work » Needs review
FileSize
8.48 KB
2.28 KB

Implementing comment #44.

Status: Needs review » Needs work

The last submitted patch, 45: select_theme_for_diff-2799597-45.patch, failed testing.

johnchque’s picture

As discussed, moved the __construct. :)

toncic’s picture

Status: Needs work » Needs review
FileSize
8.72 KB
1.98 KB

Moved construct in the right place.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/VisualDiffThemeNegotiator.php
@@ -0,0 +1,49 @@
+
+  public function __construct(AccountInterface $user,
+    ConfigFactoryInterface $config_factory,
+    EntityManagerInterface $entity_manager,
+    AdminContext $admin_context) {

This usually goes in one line.

+++ b/src/VisualDiffThemeNegotiator.php
@@ -0,0 +1,49 @@
+        if ('standard' === \Drupal::config('diff.settings')->get('general_settings.theme')) {

if we are injecting, we can avoid this and call $this->config->get... instead.

toncic’s picture

I changing the call to use $this->config()->get().

This usually goes in one line.

Yes, it is go, but break a rule of line length, if is in one line the line is long 165 characters which is double of normal. I found that is the better to use function call in multi line, and on this way I don't brake any rule of coding standard.

Status: Needs review » Needs work

The last submitted patch, 50: interdiff-2799597-48-50.patch, failed testing.

The last submitted patch, 50: select_theme_for_diff-2799597-50.patch, failed testing.

toncic’s picture

toncic’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 53: select_theme_for_diff-2799597-53.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review
FileSize
7.92 KB
4.89 KB

I actually found the problem in the tests. This should make it work. :)

toncic’s picture

IMHO this is almost the same as patch #48. For me looks good.

miro_dietiker’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Yeah, looks great.
But it's not a "theme" selection, but something specific to visual_inline.
Please prefix keys and the UI label with that reference.
Label: Theme => Visual inline theme

tduong’s picture

Some more feedbacks:

  1. +++ b/src/Form/GeneralSettingsForm.php
    @@ -137,6 +137,22 @@ class GeneralSettingsForm extends ConfigFormBase {
    +    $form['theme'] = [
    +      '#type' => 'select',
    +      '#title' => $this->t('Theme'),
    +      '#options' => [
    +        'standard' => $this->t('Standard'),
    +        'admin' => $this->t('Admin'),
    +      ],
    +      '#description' => $this->t('Theme used for Visual inline layout when comparing revisions.'),
    +      '#default_value' => $config->get('general_settings')['theme'],
    +      '#states' => [
    +        'visible' => [
    +          ':input[name="layout_plugins[visual_inline][enabled]"]' => ['checked' => TRUE]
    +        ]
    +      ],
    +    ];
    

    I would suggest to put this in an if-check to be displayed only when Visual inline layout is installed.
    Also put a comment to describe what is the check for.

  2. +++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
    @@ -8,7 +8,6 @@ use Drupal\Core\Entity\EntityInterface;
    -use Drupal\Core\Url;
    
    @@ -148,6 +147,7 @@ class VisualInlineDiffLayout extends DiffLayoutBase {
    +
    

    Unrelated changes (?)

  3. +++ b/src/Tests/AdminFormsTest.php
    @@ -37,6 +37,7 @@ class AdminFormsTest extends DiffTestBase {
    +      'theme' => 'standard',
    

    Shouldn't you test that the default theme value is standard instead of just checking that the configurations are saved correctly ?

  4. +++ b/src/VisualDiffThemeNegotiator.php
    @@ -0,0 +1,36 @@
    + * Custom front end theme.
    

    This could be improved and specified this class is about the "Visual inline" diff layout.

  5. +++ b/src/VisualDiffThemeNegotiator.php
    @@ -0,0 +1,36 @@
    +    if ($route_match->getRouteName() === 'diff.revisions_diff') {
    +      if ($route_match->getParameter('filter') === 'visual_inline') {
    +        if ($this->configFactory->get('diff.settings')->get('general_settings.theme') === 'standard') {
    

    As @Berdir mentioned at #24, the first two if-statements could be merged together.

    And why did you delete the --construct() from this class ?
    Afaik doing $this->config = $configFactory->get('diff.settings'); is fine as @toncic92 did (only reformat the construct parameters to be in a single line, for this specific case is allowed to exceed the 80 length line standards).
    I guess what @Berdir meant with "do not get the config object" is
    ->get('general_settings.theme') which is correct to be done in the method applies() to shorten this check, but ok...

  6. +++ b/src/VisualDiffThemeNegotiator.php
    @@ -0,0 +1,36 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function determineActiveTheme(RouteMatchInterface $route_match) {
    +    return $this->configFactory->get('system.theme')->get('default');
    +  }
    

    Since this is nothing new than the parent method, then you could remove it, unless you should change to return the diff visual theme one.

  7. +++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
    @@ -0,0 +1,92 @@
    +   * Testing if applies method returns true.
    ...
    +   * Check the value returned by the applies method when the theme used for
    

    According to the "Drupal API documentation standards for functions" these sentences "must start with a third person singular present tense verb".

  8. +++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
    @@ -0,0 +1,92 @@
    +    $theme = new VisualDiffThemeNegotiator($this->user->reveal(),
    +      $this->configFactory->reveal(),
    +      $this->entityManager->reveal(),
    +      $this->adminContext->reveal());
    

    Parameter in a single line here is fine.

miro_dietiker’s picture

EDIT: I see, you do conditional states for the setting. But indeed the plugin could be undefined (if the class is not available) and then we can omit it completely.

toncic’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
2.94 KB

Change theme into visual_inline_theme and some relevant changes.

Status: Needs review » Needs work

The last submitted patch, 61: select_theme_for_diff-2799597-61.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
8.12 KB
1.28 KB

I forgot to change on two places.

Status: Needs review » Needs work

The last submitted patch, 63: select_theme_for_diff-2799597-63.patch, failed testing.

toncic’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
567 bytes

Fixing test failing.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/Form/GeneralSettingsForm.php
@@ -137,6 +137,25 @@ class GeneralSettingsForm extends ConfigFormBase {
+    if ($this->diffLayoutManager->hasDefinition('visual_inline')) {

Add a comment here to explain why we add this check.

+++ b/src/Form/GeneralSettingsForm.php
@@ -137,6 +137,25 @@ class GeneralSettingsForm extends ConfigFormBase {
+
     return parent::buildForm($form, $form_state);

extra white line. There should be just one before buildForm.

+++ b/src/VisualDiffThemeNegotiator.php
@@ -0,0 +1,36 @@
+ * Visual inline front end theme.

As suggested, change this to: Visual inline layout theme negotiator.

+++ b/src/VisualDiffThemeNegotiator.php
@@ -0,0 +1,36 @@
+    if ($route_match->getRouteName() === 'diff.revisions_diff') {
+      if ($route_match->getParameter('filter') === 'visual_inline') {

As suggested above place this checks in the same line.

+++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
@@ -0,0 +1,92 @@
+ * Class VisualDiffThemeNegotiatorTest
+ * @coversDefaultClass \Drupal\diff\VisualDiffThemeNegotiator
+ *
+ * @group diff

This is usually:
Class V...

@coverDefaultCl...
@group diff

Change the white lines. :)

+++ b/tests/src/Unit/VisualDiffThemeNegotiatorTest.php
@@ -0,0 +1,92 @@
+   * Testing if applies method returns true.

As suggested above, change this to start with third person: Tests if ...

toncic’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
1.88 KB

Changes from #66.

johnchque’s picture

Status: Needs review » Reviewed & tested by the community

Works really nice!

tduong’s picture

Status: Reviewed & tested by the community » Needs work

There are still some stuff not considered in the last patch:

  1. +++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
    @@ -8,7 +8,6 @@ use Drupal\Core\Entity\EntityInterface;
    -use Drupal\Core\Url;
    
    @@ -148,6 +147,7 @@ class VisualInlineDiffLayout extends DiffLayoutBase {
    +
    

    Again unrelated changes ?

  2. +++ b/src/VisualDiffThemeNegotiator.php
    @@ -0,0 +1,36 @@
    +    if ($route_match->getRouteName() === 'diff.revisions_diff') {
    +      if ($route_match->getParameter('filter') === 'visual_inline') {
    

    These statements are not merged yet ;) EDIT: apparently a private conversation with @Berdir stated that this is fine.

  3. +++ b/src/VisualDiffThemeNegotiator.php
    @@ -0,0 +1,36 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function determineActiveTheme(RouteMatchInterface $route_match) {
    +    return $this->configFactory->get('system.theme')->get('default');
    +  }
    

    So are we going to keep this here anyway since this is just copied/pasted from parent?

toncic’s picture

Status: Needs work » Needs review
FileSize
7.39 KB
1.96 KB

So are we going to keep this here anyway since this is just copied/pasted from parent?

.
This:
return $this->configFactory->get('system.theme')->get('default');
and this:
return $this->configFactory->get('system.theme')->get('admin');

is not the same.

johnchque’s picture

Status: Needs review » Needs work
+++ b/src/Plugin/diff/Layout/VisualInlineDiffLayout.php
@@ -8,7 +8,6 @@ use Drupal\Core\Entity\EntityInterface;
-use Drupal\Core\Url;

Unrelated change.

toncic’s picture

I can left it, and someone will create new issue just to remove it?

tduong’s picture

Hah, missed that 'admin'/'default' thingy (see #70 post), sorry my bad :P

No, you should revert it because it will conflict with other cleanup issues ;)

toncic’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
605 bytes

miro_dietiker’s picture

Status: Needs review » Fixed

Committed after fixing a missing space in services.yml. Thx all!

Berdir’s picture

+++ b/src/VisualDiffThemeNegotiator.php
@@ -0,0 +1,36 @@
+    if ($route_match->getRouteName() === 'diff.revisions_diff') {

as mentioned in chat but not here: this will only work on the node route, not other entity types.

miro_dietiker’s picture

Status: Fixed » Needs work

Setting back to needs work to create a follow-up for caring about #77.

toncic’s picture

Status: Needs work » Needs review
toncic’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.