Adopt to new modern dialog

[nagmat84]

This PR makes the Lychee frontend use the revised modal dialog (already merged LycheeOrg/basicModal#3), with additional bug fixes by LycheeOrg/basicModal#5.

The advantages of using the revised modal dialog are:

• Proper support for text areas (Also a prerequisite for Markdown support #280).

• Proper support for check boxes: Their boolean values are now also part of the return object provided by the modal dialog, no need to use inefficient query selectors like in

  Lychee-front/scripts/main/album.js

  Lines 826 to 833 in ab0f314

  	// TODO: If the modal dialog would provide us with proper boolean values for the checkboxes as part of `data` the same way as it does for text inputs, then we would not need these slow and awkward jQeury selectors
  	album.json.is_nsfw = $('.basicModal .switch input[name="is_nsfw"]:checked').length === 1;
  	album.json.is_public = $('.basicModal .switch input[name="is_public"]:checked').length === 1;
  	album.json.grants_full_photo = $('.basicModal .choice input[name="grants_full_photo"]:checked').length === 1;
  	album.json.requires_link = $('.basicModal .choice input[name="requires_link"]:checked').length === 1;
  	album.json.is_downloadable = $('.basicModal .choice input[name="is_downloadable"]:checked').length === 1;
  	album.json.is_share_button_visible = $('.basicModal .choice input[name="is_share_button_visible"]:checked').length === 1;
  	album.json.has_password = $('.basicModal .choice input[name="has_password"]:checked').length === 1;

• Correct values types: Strings from input fields, numbers from input fields, booleans from checkboxes, selections from - well - selections are returned with the proper JS type.

The additional achievements of this PR on top are:

• Getting rid of lychee.html: Modal dialogs don't need the unfortunate, home-brewed escape mechanism provided by lychee.html as in

  Lychee-front/scripts/main/album.js

  Line 382 in ab0f314

  body: lychee.html`<p>${lychee.locale["TITLE_NEW_ALBUM"]} <input class='text' name='title' type='text' maxlength='100' placeholder='Title' value='${lychee.locale["UNTITLED"]}'></p>`,

  . Instead the property .textContent is used after the dialog has been created to set the text inside an element which allows to use full Unicode as JS strings are UTF-16. This ensures that no non-ASCII character in any user-provided string (title, description, etc.) can trigger errors. This is also a prerequisite for Localization #318 and Consolidate localization Lychee#1494

• The upload dialog became more robust.

  • No more inefficient and unreliable CSS selector to pick the n-th status entry to update. Instead, DOM-elements are now cached and are accessible by the key of the status message (i.e. the path of the file related to the message).
  • Scrolling to the current status entry became more reliable (for me it was always off by some pixels). Instead of calculating the position manually based on magic pixel distances as in

    Lychee-front/scripts/main/upload.js

    Lines 212 to 214 in ab0f314

    	let scrollPos = 0;
    	if (fileIdx + 1 > 4) scrollPos = (fileIdx + 1 - 4) * 40;
    	$(".basicModal .rows").scrollTop(scrollPos);

    we simply use row.listEntry.scrollIntoView(upload.SCROLL_OPTIONS); which we can now do, because we work with proper DOM elements (here: listEntry).

  Note, upload.js is still a mess with a lot of code duplication for importing photos from server and uploading via web browser. This needs to be fixed in another PR, not this one.

• Simplified structure of modal dialogs and simplified, more consistent CSS: (more about that below)

Structure of modal dialogs and CSS

The current way how dialogs were constructed was ways too complicated. Previously, a single checkbox with a label to the left and an indented description below was constructed like this

<div class='choice'>
	<label>
		<input type='checkbox' name='grants_full_photo'>
		<span class='checkbox'>${build.iconic("check")}</span>
		<span class='label'>${lychee.locale["XYZ"]}</span>
	</label>
	<p>${lychee.locale["XYZ"]}</p>
</div>

Note the redundant use of the class 'choice' in div and span. The unnecessary nesting and the additional span to build the icon for the checkbox. Now the same thing can be achieved via

<div class='input-group compact-inverse'>
	<label>${lychee.locale["XYZ"]}</label>
	<input type='checkbox' name='requires_link' />
	<p>${lychee.locale["XYZ"]}</p>
</div>

Moreover, previously every dialog was more or less a unique specimen. I went back to the drawing board and considered all dialogs Lychee uses and tried to carve out a "common structure". The result and assumptions are documented in the CSS:

Lychee-front/styles/main/_form.scss

Lines 1 to 14 in 09ce761

	// A HTML `<form>` is expected to consists of a sequence of one ore more
	// `<div>` elements.
	// Each of this `<div>` binds together a label, an input element and some
	// explanatory text.
	// This kind of `<div>` is called an input group.
	// There are several classes of input groups which define how their children
	// are arranged.
	//
	// First, this file defines styles for form elements which always apply
	// and which are independent of the class of the input group.
	// Second, this file defines the different input groups and adjusts some
	// form elements acc. to the class of input group.
	// 1. Form elements (general styling, independent of input group)

and

Lychee-front/styles/main/_form.scss

Lines 190 to 235 in 09ce761

	// 2. Input groups
	//
	// Only <div> are expected as direct children of a form.
	// These <div> constitute a so-called "input group".
	// An input group consists of the following elements in the prescribed
	// order:
	//
	// - an optional `<label>`
	// - exactly one mandatory HTML form element:
	// -- `<input>`,
	// -- `<div class='select`> nesting a `<select>`,
	// -- `<textarea>`, or
	// -- `<output>`
	// - zero or more optional `<p>` elements with a descriptive text
	// - zero or more optional `<div>` with nested input groups
	//
	// One note about `<select>`: This element is hard to style with CSS,
	// because it is treated as a replaced element. Hence, `<select>` needs to
	// be wrapped into an additional `<div>` and most styles are applied to
	// this `<div>`.
	// See: https://developer.mozilla.org/en-US/docs/Learn/Forms/Advanced_form_styling
	//
	// An input group can have one out of four layouts which determines
	// how the label, the form element, the description and the nested
	// input groups are arranged:
	//
	// 1. `stacked`
	// All elements are vertically stacked above each other.
	//
	// 2. `compact`
	// The label and the form element are on the same horizontal line.
	// The other elements follow below with a hanging indention and
	// are aligned to the form element
	//
	// 3. `compact-inverse`
	// The form element and the label are on the same horizontal line.
	// The other elements follow below with a hanging indention and are
	// aligned to the form element
	//
	// 4. `compact-no-indent`
	// The label and the form element are on the same horizontal line.
	// The other elements follow below.
	//
	// Input groups can be nested which is mostly useful in combination with
	// one of the indented layouts.
	//

This new approach and the simplified HTML makes the layout of the dialogs more robust and actually allows to nest form elements with several levels of indention (in case this is every required).

Previously, the layout worked as follows:

The actual <input> element had been sitting between the <label> which visualized the checkbox and the label to the right. It was invisible but contributed 14px of horizontal spacing to the gap in between the visual representation of the checkbox and its label. The description below has been responsible for its own indentation by 35px on the left to visually align with the label above. Hence, the CSS created an unspoken interdependence between the with of the visual representation of the checkbox (16px), the the width of the invisible real checkbox (14px) and a 1px border of the label which in sum "accidentally" happen to equal the 35px of indention of the description.

The new approach looks like this

It has become much easier as it lifts any implicit interdependence. Now the div which creates the "input group" is responsible for the correct indention which applies to all elements in the input group simultaneously. Also the <input> element is actually the input element.

[nagmat84]

"Import from Server" appears to be untested as it fails in multiple ways. I started fixing it (see my suggestions) but I just seem to uncover more bugs... In particular, method action (under server) needs to be renamed to importFromServer, but then data.paths.trim() fails because data.paths is an array, not a string. At that point I gave up as it's getting late here...

I fixed it in f181f2c.

It was indeed a failed merge. I don't know how that happened. Luckily I could restore the correct code from an older commit. @kamil4 You already found 95% of the problems yourself in a9f5ba1.

In particular, method action (under server) needs to be renamed to importFromServer, but then data.paths.trim() fails because data.paths is an array, not a string.

The code that was there was completely outdated. It suddenly also used jQuery selectors again which actually was an alarm sign to me that something really, really bad had happened. The remaining bug fix is in f181f2c. It was mostly about deleting old code which had sneaked in again.

It already had been correct in ef3eddc once. I hope this is the only "accident" which happened.

[nagmat84]

@kamil4 I solved two of the three issues which you raised on Gitter at Oct 15th 2022 07:17.

• Wrt. to auto-focus: Fixed by Automatically focus first interacive element basicModal#10. This was actually an oversight in basicModal. The commit 1211769 here is only included to illustrate the the fix for basicModal works until that has been merged upstream
• Wrt. to search box: Fixed by 3b75276. This was a problem with the specificity of CSS ruies.

[kamil4]

Somehow the commit 1211769 has no effect for me; even if I delete the whole node_modules and refetch everything using npm install, it still fetches the old version. I had to copy the files over manually. Now the focusing indeed works.

I see an undesirable change in the look in the individual photo download dialog.

Old:

New:

As you can see, the black bar on top of each button is missing. Another, more subtle change is that whereas before the buttons were subtly darker than the background, now they fully blend in. From a quick look, on master the buttons are of the more generic basicModal__button class, whereas on your branch they are button class. There's also another, though probably unrelated regression, in that the space in front of the opening parenthesis is gone.

[kamil4]

When importing from the server, any errors (e.g., if there's an index.html file in the folder) are now reported underneath the "Finished" line for the whole folder where they occurred, whereas before they would end up above. I believe I originally coded it that way with the intention that the output be chronological in nature from top to bottom, but honestly I don't have a strong preference anymore. I'm pointing it out just in case somebody else had a strong opinion...

[nagmat84]

Somehow the commit 1211769 has no effect for me; even if I delete the whole node_modules and refetch everything using npm install, it still fetches the old version.

That is somehow expected. I noticed the same problem. You must run npm update instead of npm install. The difference between both commands for JS corresponds to the difference between composer udpate and composer install for PHP. npm install only reads package-lock.json and installs the packages listed in there with their exact locked version. npm update reads package.json, resolves the dependency tree, writes a new package-lock.json for later use by npm install and then installs the packages.

However, for some reason we don't commit package-lock.json, but only package.json. Hence, npm install won't work as expected because it will always install the (potentially outdated) package version you have installed locally. I don't know why we do it that way. It is inconsistent to our approach for composer where we also commit the "locked" version. This was introduced by @ildyria in 8b6a10e.

[ildyria]

This was introduced by @ildyria in 8b6a10e.

I don't remember why I did this. 🙈

[nagmat84]

I see an undesirable change in the look in the individual photo download dialog.

[...]

As you can see, the black bar on top of each button is missing. [...] From a quick look, on master the buttons are of the more generic basicModal__button class, whereas on your branch they are button class.

The different style classes are not the cause of the regression. They have been there before and still are. The regression was rather a victim of my attempt to streamline the CSS. Do you remember the guy who was whining about the CSS being too complicated to create a custom color scheme? Same is true for everything else in this CSS.

Previously, there has been two different kind of settings at two different places which played together to achieve the former visual effect:

.downloads .basicModal__button {
	box-shadow: inset 1px 0 0 rgba(0,0,0,.2);
	border-radius: 5px;
}

.basicModal__button {
	border-top: 1px solid rgba(0,0,0,.2);
}

The first CSS rule adds a "shadow" to the left side of the box. But as the third and fourth parameter ("blur radius" and "spread radius") are both zero, the "shadow" degenerates into something which creates the same visual effect as a left border of 1px width and a 20% transparent black. The same effect could have been achieved more explicitly (and efficiently computation wise) by border-left: 1px solid rgba(0,0,0,.2). The second CSS rule adds a top border. Hence, both combined together give the appearance of a border at the left and at the top.

When I came across the second rule I just deleted it, because frankly it doesn't make much sense to have a border around a button only at the top.

This is a general problem with that whole CSS: It is a bunch of statements which are wildly scattered across several different rules but logically belong together. Even worse they depend on each other: if you change the color in one of the rules you need to remember to change to other rule, too, or the color of the border-like left shadow and the real top border won't match. It's not separation of concerns but diffusion of concerns. 😁

Anyway I fixed it in e72d7a5 but in a slightly different way than before. It is now simply:

.button {
	border: 1px solid rgba(0,0,0,.2);
	border-radius: 5px;
}

So now we have a border everywhere (top, left, bottom, right). @kamil4 Let me know if you like this one.

Honestly, I would like to throw out all the shadow statements from the CSS which do not create a real shadow but are just misused to create the visual appearance of a border. These are probably 90% of all shadow statements. But that's another PR.

Another, more subtle change is that whereas before the buttons were subtly darker than the background, now they fully blend in.

I cannot confirm that the background of the button was subtly darker than the background on master. My color picker also shows the same values. Are you sure that this is not only a visual illusion due to the darker border? But anyway, I added a rule to make the button actually a little bit darker. Now it is

.button {
	border: 1px solid rgba(0,0,0,.2);
	border-radius: 5px;
	background-color: black(0.05);
}

There's also another, though probably unrelated regression, in that the space in front of the opening parenthesis is gone.

Fixed in 30fe8df.

[nagmat84]

When importing from the server, any errors (e.g., if there's an index.html file in the folder) are now reported underneath the "Finished" line for the whole folder where they occurred, whereas before they would end up above. I believe I originally coded it that way with the intention that the output be chronological in nature from top to bottom, but honestly I don't have a strong preference anymore. I'm pointing it out just in case somebody else had a strong opinion...

@kamil4 fixed it in 79a9761. First, I thought a fix would make the code more complicated, but it actually removed lines of code. But instead we have a long comment now 😆

[kamil4]

I'll prefix this by saying that I'm not a graphic designer so I have no inherent understanding why things are the way they are or how they should be.

BUT (you knew that was coming, didn't you? 😉).

When I came across the second rule I just deleted it, because frankly it doesn't make much sense to have a border around a button only at the top.

I'm in agreement with you on this, I think. I've always found it strange how many buttons in Lychee look "unfinished", with no border at all on the bottom or the right. I'm all for doing something about that. And that applies not just to the download" buttons, but also, e.g., to "Copy to clipboard" in the photo's "direct links" dialog, or to the "key" button in the login dialog:

So now we have a border everywhere (top, left, bottom, right). @kamil4 Let me know if you like this one.

I like that there's a border. I don't like that the buttons are completely "flat" now. That may be the current fashion, but I've actually always liked Lychee's subtle 3D look, I believe achieved with horizontal darker lines and brighter lines right next to each other. I believe on master this is achieved with the following two lines?

Lychee-front/styles/main/_message.scss

Lines 84 to 85 in 6471e78

	border-top: 1px solid black(0.2);
	box-shadow: inset 0 1px 0 white(0.02);

Your change removes not only the dark shadow on the left (as you point out, it's unnecessary) but also the light one on top.

Honestly, I would like to throw out all the shadow statements from the CSS which do not create a real shadow but are just misused to create the visual appearance of a border. These are probably 90% of all shadow statements. But that's another PR.

As I said, my personal preference is to preserve the current look (i.e., not go completely "flat"). If that can be achieved without shadows, that's fine with me, but it's not immediately obvious to me how to do that.

I cannot confirm that the background of the button was subtly darker than the background on master. My color picker also shows the same values. Are you sure that this is not only a visual illusion due to the darker border?

Pretty sure; I just verified with the color picker in GIMP in my "old" image from my earlier post in this thread. I'm seeing a difference by a single shade (e.g., #414141 vs #424242), but it's there.

But anyway, I added a rule to make the button actually a little bit darker.

I'm honestly not sure about that. I think a full border serves the same purpose better. I'm for eliminating that color difference and keeping the border instead.

This is a general problem with that whole CSS: It is a bunch of statements which are wildly scattered across several different rules but logically belong together. Even worse they depend on each other: if you change the color in one of the rules you need to remember to change to other rule, too, or the color of the border-like left shadow and the real top border won't match. It's not separation of concerns but diffusion of concerns. 😁

I would call the state of Lychee CSS consistent with the state of the rest of the front end: it is spaghetti code.

You must run npm update instead of npm install

I try to avoid running npm update (and composer update) like a plague. They tend to do more than what I want. So I heartily agree with including package-lock in the git repo of the front end, like we do with the back end.

HEADS-UP: I'm going to push a commit to your branch, with a few CSS changes. Tweak as you see fit:

• Add back top shadow to the download buttons; remove the background color change.
• Add the border and shadow to the "key" button in the login dialog. For these small buttons I feel that they need the shadow both on the top and the left or they blend in with the background. I also made the button a little larger or it looked ridiculous with the full border.
• Add the border and shadow to the "copy to clipboard" buttons in "direct links". Remove the bottom compensation because at least for me it looks unaligned with that compensation in place?
• Add the border and shadow to the token dialog buttons (here the compensation looks just fine, BTW).

This should take care of the dialog boxes. There are more "unfinished" buttons in Settings, Users, etc., but that's a separate problem.

[kamil4]

I finished going over my notes from the code review of this PR and I'm satisfied with its state at this point (though see two notes about missing localizations that I just added).

[kamil4]

I believe this string should be localized?

[nagmat84]

Fixed in 069b906.

[kamil4]

Same comment regarding the localization of "General".

[nagmat84]

Fixed in 069b906.