Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#74 | interdiff-2799597-70-74.txt | 605 bytes | toncic |
#74 | select_theme_for_diff-2799597-74.patch | 6.8 KB | toncic |
| |||
#70 | interdiff-2799597-67-70.txt | 1.96 KB | toncic |
#70 | select_theme_for_diff-2799597-70.patch | 7.39 KB | toncic |
| |||
#67 | interdiff-2799597-65-67.txt | 1.88 KB | toncic |
Comments
Comment #2
Berdironly 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.
Comment #3
toncic CreditAttribution: toncic at MD Systems GmbH commentedTrying to added theme for visual_inline layout.
Comment #4
toncic CreditAttribution: toncic at MD Systems GmbH commentedProviding screenshot.
Comment #5
Berdirnever 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.
Comment #6
toncic CreditAttribution: toncic at MD Systems GmbH commentedImplemented comm #5.
Comment #7
johnchqueI like it really much, but:
here we need a prefix and suffix as the other dropdown buttons.
Something like:
and we need to use code like:
to set the current used theme as the default in the button.
Comment #8
toncic CreditAttribution: toncic at MD Systems GmbH commentedApplying comm #7.
Comment #9
johnchquedo 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.
Comment #10
johnchquemissing "="
Comment #11
miro_dietikerOh, 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.
Comment #12
toncic CreditAttribution: toncic at MD Systems GmbH commentedFixing typo and coding standard.
Comment #13
toncic CreditAttribution: toncic at MD Systems GmbH commentedComment #14
miro_dietikerYou still didn't move the selection to the admin settings.
And tests. :-)
Comment #15
toncic CreditAttribution: toncic at MD Systems GmbH commentedMoved button into general settings and test coverage.
Comment #17
johnchqueWorks niiiiiice, but
add a
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.
Comment #18
toncic CreditAttribution: toncic at MD Systems GmbH commentedI am not sure that I added default theme on the right way.
Providing screenshots.
Comment #21
toncic CreditAttribution: toncic at MD Systems GmbH commentedSorry for interdiff.
Comment #22
toncic CreditAttribution: toncic at MD Systems GmbH commentedFixing test fails.
Comment #23
johnchqueI think this can be nicer if says something like: "Theme used for Visual inline layout when comparing revisions."
Comment #24
BerdirDon't think that is a very good name. Maybe something like VisualDiffThemeNegotiator since it is specifically about that?
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.
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)
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?
Comment #25
toncic CreditAttribution: toncic at MD Systems GmbH commentedAdded test coverage for functionality and changes from #24.
Comment #27
johnchqueWhy back_end?
As discussed, change this name.
Comment #28
toncic CreditAttribution: toncic at MD Systems GmbH commentedImplement comm #27.
Comment #30
toncic CreditAttribution: toncic at MD Systems GmbH commentedPatch rebased and small clean patch.
Comment #32
toncic CreditAttribution: toncic at MD Systems GmbH commentedUploaded test without test.
Comment #34
toncic CreditAttribution: toncic at MD Systems GmbH commentedTrying one more time without test for functionality.
Comment #35
johnchque"Negotiator" right?
Comment #36
toncic CreditAttribution: toncic at MD Systems GmbH commentedAdded test for functionality.
Comment #39
toncic CreditAttribution: toncic at MD Systems GmbH commentedAdded @group annotation.
Comment #41
toncic CreditAttribution: toncic at MD Systems GmbH commentedChanged @group.
Comment #43
toncic CreditAttribution: toncic at MD Systems GmbH commentedThis should works.
Comment #44
johnchqueIt might be worth to add some comment here, at least in the assertTrue.
Comment #45
toncic CreditAttribution: toncic at MD Systems GmbH commentedImplementing comment #44.
Comment #47
johnchqueAs discussed, moved the __construct. :)
Comment #48
toncic CreditAttribution: toncic at MD Systems GmbH commentedMoved construct in the right place.
Comment #49
johnchqueThis usually goes in one line.
if we are injecting, we can avoid this and call $this->config->get... instead.
Comment #50
toncic CreditAttribution: toncic at MD Systems GmbH commentedI changing the call to use $this->config()->get().
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.
Comment #53
toncic CreditAttribution: toncic at MD Systems GmbH commentedRebase patch.
Comment #54
toncic CreditAttribution: toncic at MD Systems GmbH commentedComment #56
johnchqueI actually found the problem in the tests. This should make it work. :)
Comment #57
toncic CreditAttribution: toncic at MD Systems GmbH commentedIMHO this is almost the same as patch #48. For me looks good.
Comment #58
miro_dietikerYeah, 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
Comment #59
tduong CreditAttribution: tduong at MD Systems GmbH commentedSome more feedbacks:
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.
Unrelated changes (?)
Shouldn't you test that the default theme value is
standard
instead of just checking that the configurations are saved correctly ?This could be improved and specified this class is about the "Visual inline" diff layout.
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 methodapplies()
to shorten this check, but ok...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.
According to the "Drupal API documentation standards for functions" these sentences "must start with a third person singular present tense verb".
Parameter in a single line here is fine.
Comment #60
miro_dietikerEDIT: 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.
Comment #61
toncic CreditAttribution: toncic at MD Systems GmbH commentedChange theme into visual_inline_theme and some relevant changes.
Comment #63
toncic CreditAttribution: toncic at MD Systems GmbH commentedI forgot to change on two places.
Comment #65
toncic CreditAttribution: toncic at MD Systems GmbH commentedFixing test failing.
Comment #66
johnchqueAdd a comment here to explain why we add this check.
extra white line. There should be just one before buildForm.
As suggested, change this to: Visual inline layout theme negotiator.
As suggested above place this checks in the same line.
This is usually:
Class V...
@coverDefaultCl...
@group diff
Change the white lines. :)
As suggested above, change this to start with third person: Tests if ...
Comment #67
toncic CreditAttribution: toncic at MD Systems GmbH commentedChanges from #66.
Comment #68
johnchqueWorks really nice!
Comment #69
tduong CreditAttribution: tduong at MD Systems GmbH commentedThere are still some stuff not considered in the last patch:
Again unrelated changes ?
These statements are not merged yet ;) EDIT: apparently a private conversation with @Berdir stated that this is fine.
So are we going to keep this here anyway since this is just copied/pasted from parent?
Comment #70
toncic CreditAttribution: toncic at MD Systems GmbH commented.
This:
return $this->configFactory->get('system.theme')->get('default');
and this:
return $this->configFactory->get('system.theme')->get('admin');
is not the same.
Comment #71
johnchqueUnrelated change.
Comment #72
toncic CreditAttribution: toncic at MD Systems GmbH commentedI can left it, and someone will create new issue just to remove it?
Comment #73
tduong CreditAttribution: tduong at MD Systems GmbH commentedHah, 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 ;)
Comment #74
toncic CreditAttribution: toncic at MD Systems GmbH commentedComment #76
miro_dietikerCommitted after fixing a missing space in services.yml. Thx all!
Comment #77
Berdiras mentioned in chat but not here: this will only work on the node route, not other entity types.
Comment #78
miro_dietikerSetting back to needs work to create a follow-up for caring about #77.
Comment #79
toncic CreditAttribution: toncic at MD Systems GmbH commentedAdded follow up #2810267: Make theme negotiator works with other entity types
Comment #80
toncic CreditAttribution: toncic at MD Systems GmbH commented