Conflicting Files ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
16 Jan 2021

Pull Request for pending todos

Summary of Changes

  • Create a new renderer bodybottom which renders just before the closing body tag

  • Add a new private variable to the Document

  • Add setter/getter for the previous private variable

  • Fix the @todos for the toolbar modals

Testing Instructions

Apply patch, and check that the modals in the Article Edit form (both the Versions and Preview) are still functional

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

@wilsonge although the problem was pretty obvious in the BS5 transition this solution and the shenanigans (the ability to enqueue html for the bottom of the page) should be pretty valid for J4 regardless of the BS5 decision

avatar dgrammatiko dgrammatiko - open - 16 Jan 2021
avatar dgrammatiko dgrammatiko - change - 16 Jan 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jan 2021
Category Layout Libraries
c26d9a4 16 Jan 2021 avatar dgrammatiko init
avatar dgrammatiko dgrammatiko - change - 16 Jan 2021
Labels Added: ?
avatar dgrammatiko dgrammatiko - change - 16 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 16 Jan 2021
avatar brianteeman
brianteeman - comment - 16 Jan 2021

If you can think of a better word than queue it will be better in the long term as its so easy to make a typo with it

avatar dgrammatiko
dgrammatiko - comment - 16 Jan 2021

If you can think of a better word than queue it will be better in the long term as its so easy to make a typo with it

I'm not good at naming things but I'm open to anything here

a80e314 16 Jan 2021 avatar dgrammatiko oops
406f18d 16 Jan 2021 avatar dgrammatiko Meh
avatar SharkyKZ
SharkyKZ - comment - 16 Jan 2021

This produces invalid HTML.

avatar dgrammatiko
dgrammatiko - comment - 17 Jan 2021

This produces invalid HTML.

Interested to see how you came up with this conclusion

Fixed

avatar ITPrism
ITPrism - comment - 17 Jan 2021

It is a good idea but it will be better if the assets are managed by the assets manager.
We can provide an option position with values bodystart and bodyend. We should rework the script render to parser jdoc blocks depending on the attribute position.

Factory::getDocument()->getWebAssetManager()
	->addInlineScript(
		HTMLHelper::_('bootstrap.renderModal',
			'modal_' . $selector,
			[
				'url'         => $displayData['doTask'],
				'title'       => $text,
				'height'      => '100%',
				'width'       => '100%',
				'modalWidth'  => 80,
				'bodyHeight'  => 60,
				'closeButton' => true,
				'footer'      => '<button class="btn btn-secondary" data-bs-dismiss="modal" type="button"'
				        . ' onclick="window.parent.Joomla.Modal.getCurrent().close();">'
				        . Text::_('COM_BANNERS_CANCEL') . '</button>'
				        . '<button class="btn btn-success" type="button"'
				        . ' onclick="Joomla.iframeButtonClick({iframeSelector: \'#modal_downloadModal\', buttonSelector: \'#exportBtn\'})">'
				        . Text::_('COM_BANNERS_TRACKS_EXPORT') . '</button>',
			]
		),
		['position' => 'bodyend']
	);
<head>
     <jdoc:include type="scripts" />
</head>

<body>
     <jdoc:include type="scripts" position="bodystart" />
      .....
      ....
     <jdoc:include type="scripts" position="bodyend" />
</body>
avatar dgrammatiko
dgrammatiko - comment - 17 Jan 2021

It is a good idea but it will be better if the assets are managed by the assets manager.

This PR is not affecting the script position, it's a way to delegate a chunk of HTML to the end of the document.

Could the scripts refactored to take advantage of the new position?

Definitely but that's for another PR. This one solves a problem that all the Toolbar buttons that trigger a Modal have issues with the z-index (conflicting dropdown and modal) so the rendered Modal HTML needs to be delegated to another place in the document (and since they are Modals could happily exists as the last elements in the page)

Hope this clears things up

PS the HTMLHelper::_('bootstrap.renderModal', []) returns an HTML string not a script ?

b318bbb 19 Jan 2021 avatar dgrammatiko CS
5e275e5 19 Jan 2021 avatar dgrammatiko CS
avatar chnnst
chnnst - comment - 22 Jan 2021

What still needs to be done to merge this PR?

avatar dgrammatiko
dgrammatiko - comment - 22 Jan 2021

What still needs to be done to merge this PR?

It needs 2 successful tests as every other Pull Request

avatar chnnst
chnnst - comment - 22 Jan 2021

I see 3 successful checks for this PR, thats why I asking what still needs to be done to merge this PR?

avatar dgrammatiko
dgrammatiko - comment - 22 Jan 2021

I see 3 successful checks for this PR, thats why I asking what still needs to be done to merge this PR?

Thumbs up is not the way a test result is counted. You need to use https://issues.joomla.org/tracker/joomla-cms/32062 to mark your test as successful or failed

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

I guess this is not going anywhere

avatar dgrammatiko dgrammatiko - change - 2 Feb 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-02-02 23:09:01
Closed_By dgrammatiko
Labels Added: Conflicting Files
avatar dgrammatiko dgrammatiko - close - 2 Feb 2021
avatar chnnst
chnnst - comment - 3 Feb 2021

Why you closed this pull?

avatar dgrammatiko
dgrammatiko - comment - 3 Feb 2021

Two reasons:

  • no tests after 2+ weeks
  • this has an odd edge case
avatar chnnst
chnnst - comment - 3 Feb 2021

for test you should leave open this

for edge case I don't know

Add a Comment

Login with GitHub to post a comment