WICKET-6666 initial checkin of new ModalDialog #361

ivaynberg · 2019-05-10T20:43:21Z

I dont think this is ready to be merged, its just here to start a conversation...

andruhon

Looks better than the old one. I'll have a closer look tomorrow.

andruhon · 2019-05-13T13:08:47Z

...-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.js

+	// Entry Methods
+	//
+
+	window.wicket = window.wicket || {};


Why Wicket ns doesn't start from capital W here?

no particular reason.

We should change it to capital W. The idea here is to have one global entry in the JS DOM - window.Wicket

andruhon

I believe that some abstract component such as AbstractModalDialog should be created in core to represent dialogs, it could be squeezed in as a base class for existing ModalWindow and maybe for this new one, to make it easier to switch between implementations. Potentially this abstract component can have some very basic JS with enough hooks and callbacks so it can easily be extended with other wicket extensions or replaced completely.

Also as PS license header and XML tests are failing with this change.

andruhon · 2019-05-13T20:54:30Z

...src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialogReferences.java

+	public static final ResourceReference CSS = new CssResourceReference(
+		ModalDialogReferences.class, "ModalDialog.css");
+
+	public static final ResourceReference CSS_SKIN = new CssResourceReference(


Is this CSS_SKIN optional and extends the CSS from above? Maybe leave comments for these constants explaining them. For example, I presume the JS is a required part.

the skin is the visual part of the css. in the previous modal both visual and layout css were rolled into a single file so it was harder to override what the modal looked like in applications. if you notice the dialog itself only includes the non-skin css, the skin css is included manually on the example page and serves as a starting point for applications to build their own

andruhon · 2019-05-14T00:58:54Z

Another thing I really want it to do is to work nicely with the Select2 (maybe it already does). Select2 inside of the old ModalWindow was a pain for quite a while.

ivaynberg · 2019-05-14T02:45:21Z

it already supports select2

ivaynberg · 2019-05-14T02:49:23Z

I believe that some abstract component such as AbstractModalDialog should be created in core to represent dialogs, it could be squeezed in as a base class for existing ModalWindow and maybe for this new one, to make it easier to switch between implementations. Potentially this abstract component can have some very basic JS with enough hooks and callbacks so it can easily be extended with other wicket extensions or replaced completely.

I doubt that is possible. Each dialog works in fundamentally different ways, so a base class makes little sense. The only reason why a separate class exists is to make migration easier, eventually ModalWindow will be removed.

andruhon · 2019-05-14T02:51:10Z

...sions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog-skin.css

+.modal-dialog {
+	max-width: 800px;
+	background: white;
+	box-shadow: 0 0 60px 10px rgba(0, 0, 0, 0.7);


Just my opinion: I'd say 60px is too strong for default style. Maybe 0 0 20px 0 rgba(0,0,0,0.7); ?

andruhon · 2019-05-14T02:54:25Z

Just tested the modal in Chrome, FF, Edge and IE11 all good. Looks a bit dodgy in IE11.

I doubt that is possible. Each dialog works in fundamentally different ways, so a base class makes little sense. The only reason why a separate class exists is to make migration easier, eventually ModalWindow will be removed.

I agree, old one has quite inconsistent naming it does irrelevant show/close, instead of antonyms pairs such as show/hide or open/close any many other strange things. Maybe only a new one can extend such an abstract class.

martin-g · 2019-05-14T05:29:10Z

...extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.css

+	align-items: center;
+	justify-content: center;
+	flex-direction: column;
+	z-index: 1000000;


should z-index be configurable ? AFAIK it is configurable in ModalWindow because of Select widgets.

martin-g · 2019-05-14T05:29:52Z

...extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.css

+	width: 100%;
+	margin: auto;
+	overflow: hidden;
+	z-index: 1000010;


or may be this one should be configurable ? Or both ?!

neither should be configurable from java imho, they will be overridden by application-specific css. eg we have this in our css:

.modal-dialog-overlay { z-index:3 !important; & ~ .select2-drop-mask { z-index:3; } & ~ .select2-drop { z-index:4; //make sure select2 dropdown is visible inside a modal } }

martin-g · 2019-05-14T05:31:04Z

...xtensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.java

+		// forcing this form - which will be ripped out of the dom along with the content form if
+		// there is one to always
+		// render its tag as 'form' instead of 'div'.
+		var form = new Form<Void>("form")


🍾 I believe this is the first usage of var in Wicket ! :-)

martin-g · 2019-05-14T05:34:51Z

...xtensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.java

+		container.setVisible(false);
+		add(container);
+
+		// We need this here in case the modal itself is placed inside a form. If that is the case


can we do this in onInitialize() and add it only if there is / is not an outer form ?

what would you gain? if you are not in a form you would have to add a webmarkupcontainer instead with overridden oncompoennttag that mutates the form tag to a div in markup. it seems like needless complexity to me...

Actually the question should be "Why do we need a Form here at all ?"
Why not let the application provide a Form inside the content container if it needs one or use the outer (if there is such) ?
I am on my mobile now, so I cannot re-read ModalWindow's javadoc. But it would be good to simplify here as much as possible. AFAIR Wicket-Bootstrap's Modal component does not provide such mandatory Form and no one complained.

suppose

form(1) -> modal -> content -> form(2) -> submit

because modal is a child of form(1) form(2) will render its tag as a div because html does not support nested forms

then it is moved into its own direct child of body by modal js

so now submit button does not have a form tag to work with

martin-g · 2019-05-14T05:37:53Z

...xtensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.java

+
+		this.options = options;
+
+		target.prependJavaScript(getOpenJavascript());


I would expect to use appendJavaScript() here. I guess it works with prepend because of setTimeout(..., 0) below. Should we use append to be more clear ?

the prepend is purposeful. if there are any other javascript contributed that works with elements that are inside the modal prepend guarantees that said javascript will execute after the affected elements are in the right place in the dom. i will add javadoc explaining this...

I have to double check but I think the order is:

prepend JS

replace DOM

append JS

martin-g · 2019-05-14T05:39:57Z

...-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.js

+
+	/** Retreives id of the element, creates one if none */
+	var getOrCreateIdCounter = 0;
+	function getOrCreateId(element) {


if element is a jQuery object then let's name it $element to make it more clear.

martin-g · 2019-05-14T05:43:14Z

...-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.js

+			data.opener = document.activeElement;
+			if (data.options.validate) {
+				if (!data.opener || $(data.opener).is("body")) {
+					data.options.console.error("Error saving focused element when opening the modal, it is either none or body: ",


What is the idea behind options.console ? To be able to use either window.console, Wicket.Log or a third party one ?

yes, and also to be able to suppress any warnings/errors in production build

martin-g · 2019-05-14T05:44:57Z

...-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.js

+			}
+			if (data.options.closeOnEscape) {
+				overlay.on("keydown", function (event) {
+					if (event.which == 27) {


JSHint (once ModalDialog.js is added to testing/wicket-js-tests/Gruntfile.js) will complain here on ==. We should use === instead.

svenmeier · 2019-05-14T11:26:39Z

Thanks Igor, it's much easier to start with something instead of starting from scratch.

I would like to improve some general points:

body>overlay>form>content
ModalDialog replicates what ModalWindow has done: its JS creates some wrapping HTML, moves the dialog's content inside of it and appends everything to the body. I think it would be simpler to leave the dialog where it is in the DOM: then we don't have to create markup from JS, no extra form is required and it works without JavaScript too (yes, call me old-school).
behaviors
ModalDialog has its own pure-JS 'behavior' solution. We can make these Wicket-behaviors, so they are modular and reuasable: some projects will not need all of them, want to replace them with their own or add custom ones (e.g. resizing).
styling
ModalDialog offers sort of a default skin: I'd like to make this a behavior with a base CSS selector (similar to 'themes' of Palette and AbstractTree) - this makes it possible to use different styles on a single page simultaneously (e.g. a simple warning dialog vs. an larger one with an input form).
multiple/nested dialogs
Many elements are modified via CSS class selector, we'll have to improve that to support multiple dialogs.
options.console
IMHO we can simply call Wicket.Log.* (see recent "js-cleanup" discussion)

ivaynberg · 2019-05-14T15:45:26Z

Thanks Igor, it's much easier to start with something instead of starting from scratch.

I would like to improve some general points:

body>overlay>form>content
ModalDialog replicates what ModalWindow has done: its JS creates some wrapping HTML, moves the dialog's content inside of it and appends everything to the body. I think it would be simpler to leave the dialog where it is in the DOM: then we don't have to create markup from JS, no extra form is required and it works without JavaScript too (yes, call me old-school).

that is what i started with, but i quickly ran into all sorts of zindex and other problems with more complex layouts. unfortunately there is no substitute for being the last child of body yet.

behaviors
ModalDialog has its own pure-JS 'behavior' solution. We can make these Wicket-behaviors, so they are modular and reuasable: some projects will not need all of them, want to replace them with their own or add custom ones (e.g. resizing).

im not sure these should be refactored into wicket Behaviors, these are only there to keep the code easy to understand since most things happen in open/close having everything there was messy. and they have lifecycles specific to the modal.

if customization is required we could move the behaviors array into options and allow users to provide their own

styling
ModalDialog offers sort of a default skin: I'd like to make this a behavior with a base CSS selector (similar to 'themes' of Palette and AbstractTree) - this makes it possible to use different styles on a single page simultaneously (e.g. a simple warning dialog vs. an larger one with an input form).

i am not sure how necessary this is. unlike the old modal window, this one contains nothing in its body, so you can already create different title bars for warnings/errors/other dialogs by plopping a different panel into the content.

the only thing that the css provides is the look and feel of the mask and borders which i dont think would change based on your usecase.

but, i do not feel strongly about it so feel free to tweak, the code is in a branch. my only word of caution is that a big reason why the old modal window was such a mess and so hard to maintain is because we put a ton of features into it that only a small percentage of people used, i would try to keep this one as thin as possible.

multiple/nested dialogs
Many elements are modified via CSS class selector, we'll have to improve that to support multiple dialogs.

most selectors are used downstream of the overlay element, since each overlay will be a direct child of body i dont think there will be collissions... maybe i am wrong

options.console
IMHO we can simply call Wicket.Log.* (see recent "js-cleanup" discussion)

feel free to make this change, we should also remove it from options then...

andruhon · 2019-05-15T01:14:15Z

...-extensions/src/main/java/org/apache/wicket/extensions/ajax/markup/html/modal/ModalDialog.js

 			}
 			if (data.options.closeOnEscape) {
 				overlay.on("keydown", function (event) {
-					if (event.which == 27) {


By the way which is deprecated https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/which, key is recommended instead https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

svenmeier · 2019-05-15T14:42:35Z

Hi Igor,

my only word of caution is that a big reason why the old modal window was such a mess and so hard to maintain is because we put a ton of features into it that only a small percentage of people used, i would try to keep this one as thin as possible.

I fully agree, most projects will have their special requirements anyway. This is why the new ModalDialog should rather be a "best practice" implementation instead of a "one fits all" solution.

Sven

svenmeier · 2019-05-15T15:10:09Z

Regarding the location of the dialog in the DOM - in case of a dialog nested inside a form, a page refresh produces invalid markup with two nested form-tags.
Firefox removes the inner form-tag, so after moving the dialog markup to the body, its content is no longer wrapped inside a form-tag :(.

IMHO we should restrain from such tinkering - BTW Bootstrap leaves the dialog where it is in the DOM too.

andruhon · 2019-05-15T22:35:13Z

Another concern is a presence of jQuery. It doesn't do much here now. As far as I remember Wicket has dropped support for IE9 and IE10 in v8. I think the only use for jQuery here is https://developer.mozilla.org/en-US/docs/Web/API/Element/closest if we still support the IE11 (can be polyfilled). closest works fine in MS Edge.

svenmeier · 2019-05-16T08:55:55Z

most selectors are used downstream of the overlay element

I've double checked, your right :)

svenmeier · 2019-12-09T17:15:34Z

@ivaynberg any objections against continuing the new modal dialog implementation on #375 ?
If not, could you please close this pull request?

ivaynberg · 2019-12-09T21:48:57Z

no objections

WICKET-6666 initial checkin of new ModalDialog

0460768

andruhon reviewed May 13, 2019

View reviewed changes

andruhon reviewed May 14, 2019

View reviewed changes

martin-g reviewed May 14, 2019

View reviewed changes

WICKET-6666 tweaks based on feedback from pull request

029f6f0

andruhon reviewed May 15, 2019

View reviewed changes

ivaynberg closed this Dec 9, 2019

solomax deleted the modal-dialog branch May 5, 2022 02:46


		this.options = options;

		target.prependJavaScript(getOpenJavascript());

WICKET-6666 initial checkin of new ModalDialog #361

WICKET-6666 initial checkin of new ModalDialog #361

Uh oh!

Conversation

ivaynberg commented May 10, 2019

Uh oh!

andruhon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andruhon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andruhon commented May 14, 2019

Uh oh!

ivaynberg commented May 14, 2019

Uh oh!

ivaynberg commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andruhon commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenmeier commented May 14, 2019

Uh oh!

ivaynberg commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenmeier commented May 15, 2019

Uh oh!

svenmeier commented May 15, 2019

Uh oh!

andruhon commented May 15, 2019

Uh oh!

svenmeier commented May 16, 2019

Uh oh!

andruhon left a comment •

edited

Loading

andruhon commented May 14, 2019 •

edited

Loading