[Port to 3.x] Fix Various Issues with Form Panel Validation and Field Display - Revised Submission

[smg6511]

NOTE: This PR lays the groundwork for improvements to the element creation UIs, which will be submitted after implementation of this PR. This (and each related PR to follow) is a revised submission of #15887.

What does it do?

Ports enhancements to the Resource form panel from PR #15146 to 3.x (including subsequent fixes by others).
Changes include:

1. Added/altered methods in the main FormPanel class to fix issues with/optimize validation — specifically changing more reliably to the (first) errored tab when a field it contains in invalid — which apply to the Resource panel and all Element panels (to come in related PRs momentarily).
2. Addressed a concern raised by @Ruslan-Aleev in UX inaccuracies in TV creation forms #15854 : (7) made style updates so only the settings-type checkboxes in the resource editing form display in the new switch style by default (TVs show the normal checkbox; a new input option setting could be created to control this by assigning the class I added in the forms SCSS ["display-switch"] in a future PR).
3. A couple minor lexicon updates to fix capitalization and make the Summary label read simply “Summary” instead of “Summary (introtext).”

Why is it needed?

Makes needed UI and consistency improvements to the resource panel.

How to test

1. From your terminal app, run grunt build from within the _build/templates/default folder and clear your browser cache.
2. Create at least two TVs (at least one checkbox TV and one TV of any type that is required) and verify they operate and appear as expected.
3. Move at least one required TV via Form Customization and assign it to a template. From a resource using this template, verify that (when this TV is not filled and you try to save the resource while viewing a tab other than the one containing this TV) errored tabs are opened and navigated to on save.

Related issue(s)/PR(s)

Partly addresses #15854
Partial port of #15146

[opengeek]

Whoa! I don't think this is necessary. First of all, multi-part PRs that are dependent on other PRs being merged is an absolute nightmare to manage as an integrator. The rebase vs. merge comment was just a suggestion to help keep things cleaner.

[opengeek]

I would much rather see you add the other parts as separate commits here to this PR.

[smg6511]

@opengeek - OK, will do. I'll commit each part within this PR. I think now I'll know the ideal way to submit these medium to large PRs ;-)

I'm more so re-submitting this differently because my original PR seems to not be gaining traction and that some of these changes have not been merged is beginning to make other changes I'm looking to submit increasingly tricky because they build upon or intersect in ways with the changes made in this PR. Hoping reorganizing will make reviewers more apt to review this. (I'm not griping by any means, as I realize there are a lot of PRs to review any very limited resources reviewer-wise, just explaining.)

Just note that regardless, this first commit will be required for the others to work properly.

[Ruslan-Aleev]

@opengeek The problem is that the original PR solves problems that can be divided into several independent PRs.
For example, UX problems, lexicons, processors and regular expressions, etc. сorrected in #15887 (see modified files). That is, even with split commits, the changes are too global.
It seems to me that it is worth deviding it into several parallel and independent PRs, where it possible.
It will be easier to test and merge separately.

[opengeek]

@opengeek The problem is that the original PR solves problems that can be divided into several independent PRs.
For example, UX problems, lexicons, processors and regular expressions, etc. сorrected in #15887 (see modified files). That is, even with split commits, the changes are too global.
It seems to me that it is worth deviding it into several parallel and independent PRs, where it possible.
It will be easier to test and merge separately.

But they are not independent, according to the description. Separate PRs with dependencies on other PRs is not viable.

[Ruslan-Aleev]

But they are not independent, according to the description. Separate PRs with dependencies on other PRs is not viable.

Even based on the description, they are.
Item 2 - fixes UX in the new TV form.
Item 3 - adds improvements.

This is not counting other edits.

[smg6511]

@Ruslan-Aleev - If you're looking at the original PR, you need to look at the whole picture and how the changes are implemented. No matter what, the changes I made to the FormPanel class are necessary for any of the changes to the individual panels to work. I think you're geared to wanting to see things submitted by type of change (UX, lexicon, style, etc.) but that sometimes is not the most logical way to go IMO. In this case, doesn't it make more sense to split it by the area affected (resource, and each element panel)?

[Ruslan-Aleev]

@smg6511 Such global changes bother me.
For example, a PR from TV #15773, which, in my opinion, was merged without much thought, created at least 2 issues (#15947 and #15854). And I'm sure that when MODX 3 is released and people start working with TV as usual - there will only be more issues, I'm ready to make a bet :)
That's why I propose to divide it into independent PRs, which affect less code and are easier to test.

But maybe I worry in vain, let the majority decide.

[smg6511]

For example, a PR from TV #15773, which, in my opinion, was merged without much thought, created at least 2 issues (#15947 and #15854)

@Ruslan-Aleev - In all fairness, re #15947, yes that was a miss on my part (but easily solved); #15854 is by and large problems in your opinion and not bugs. Hopefully you've seen that I'm taking your opinion seriously and am willing to compromise and respond to your and others' concerns. That said, when you refer to #15773, if you looked at it closely you might agree that it would not have been doable, practically at least, in a non global way. Yes, I've learned some commit strategies since that could have made it somewhat easier to review, but it's admittedly a beast — after all it fundamentally changed how the UI for TV creation is built and opened it up to easily overriding by custom TV creators.

Anyway, maybe we can come to a middle ground for both you and @opengeek on this particular PR's intended changes. I can leave this revised PR as-is, ready to review. (I'll of course close the original #15887.) I can wait to submit separate PRs for each element until after this PR is merged. This is possible because while my upcoming changes to each element's form panel rely on the changes in this PR, the element panels as they stand will not be affected by this PR. How does that sound?

[Ruslan-Aleev]

I can wait to submit separate PRs for each element until after this PR is merged. This is possible because while my upcoming changes to each element's form panel rely on the changes in this PR, the element panels as they stand will not be affected by this PR. How does that sound?

If it's easier to test and track changes in files, then I'm all for it.

[JoshuaLuckers]

My availability is limited, that's why I prefer smaller PR's or PR's with good commits.
Bigger PR's tend to have more discussion and stay open for much longer than needed.

But for now, let's focus on the issues and features this PR introduces instead of code style.

@smg6511 is this still WIP or ready for review?

[smg6511]

@JoshuaLuckers - Yes, this revision is ready to review. I broke up the original in hopes that it would be more approachable. I've got the changes to each element panel waiting in the wings (prepared but not submitted) ready to go after this PR is presumably merged.

[smg6511]

@mishantrop - For the purposes of this PR, I've set aside your suggestions for now. That's not to say you don't have some good/valid ones; it's just that they're nitpicks that are inconsequential to the PR's functionality. And, there's one thing I've been learning as I've become more active in submitting fixes and enhancements: With so few reviewers (i.e., testers) it's best to keep the number of commits to a minimum when possible. Believe me, there's a ton of the types of changes you suggest to be done across the whole codebase, and I'd love to fix them all at once ... but then we'd never get anything through review!

Anyway, as I've suggested before, it would be way more helpful if you could review and test PRs you're looking at, rather than just identifying code style/format to improve. It's an ideal I appreciate, but your time would be much more valuable verifying PR functionality.

[JoshuaLuckers]

From a feature standpoint this work as described!

[Ruslan-Aleev]

1. I confused you, with this checkboxes and switches in TV.
   BUT my complaint was in the inconsistency of the interfaces of the forms.
   Those previously in the TV form you made checkboxes in switches style, but for other forms, for example, chunks, they remained in the form of checkboxes.
   And in this PR, you did the opposite :) In TV, you returned checkboxes, and in other forms, for example, chunks, they are now in switches style. Why, what is the logic?
   It is worth bringing to one style. Make it in the style of switches for all forms (checkboxes should be left only in grids, I think).

TV form:

Chunk form:

2. I understand correctly that the validation improvement that we discussed earlier (Improve form validation #16009 (comment)) will be in another PR and is not fixed in this one, right?

[smg6511]

@Ruslan-Aleev - I didn't misunderstand, it's just that I broke up the whole change into multiple PRs because I keep getting grief over how big some of my PRs, including the original version of this one, are ;-)

That said, I did not submit the other PRs yet, which apply all of the same stylistic and functionality changes as this one to each of the Element editing panels, because they need some of the changes made in this PR to work. Those PRs are all prepared and ready to push though.

On question 2, yes.

[Ruslan-Aleev]

@smg6511 Then I do not understand you :)
In the current version of MODX 3 (not considering this PR) in TV checkboxes in the form of switches, in this PR you override this for TV forms, but adding the form of switches to other forms. What for?

p.s. It is also worth making PRs independent in the future, otherwise, due to the fact that one PR is not tested / merged, dependent PRs will not be merged either (because of this we have an inconsistency in the interface).

[smg6511]

What I need here is a little faith that once this PR merges (presumably), the others that follow will make everything consistent. I'm looking into the reversion in the TV form, which should not be happening.

These PRs can not be independent. There are methods added to the panels’ parent class that makes certain parts work in each. It doesn't make sense to keep re-committing that one changed class to make these PRs fully independent, right? That's why I submitted the change as one PR initially. We can’t have it both ways (unless there’s a strategy I’m missing).

[smg6511]

@Ruslan-Aleev - Ok, I think I know what could make you more comfortable with moving ahead with this PR. I can remove (for now) the css selector that is used to keep the Resource TV checkboxes normal style and not switch style. The result would be that all checkboxes will be switch style in the form panels (again temporarily). To illustrate, the _forms.scss file at line 845 would get changed to:

/* .x-form-check-wrap */
/* Special checboxes for resources and tv configs */
#modx-chunk-tabs,
#modx-plugin-tabs,
// #modx-resource-tabs .display-switch,
#modx-resource-tabs,
#modx-snippet-tabs,
#modx-template-tabs,
// #modx-tv-tabs .display-switch,
#modx-tv-tabs,
#modx-tv-editor-tabs {
[...]
}

I commented out the 2 more specific selectors that differentiate the switch/regular style. I'd like to keep those commented out lines as an extra reminder that I need to change them back after the full set of PRs is implemented.

What do you think? I'll make one more commit to do the above if you like the idea.

[Ruslan-Aleev]

@smg6511 Yes, that's a good idea, thanks, I'll test it locally in the next few days and get back to you.

[rthrash]

@Ruslan-Aleev how'd it check out on your end?

[Ruslan-Aleev]

@rthrash I completely forgot about this PR, I'll test it and write back tomorrow.

[Ruslan-Aleev]

The interface is now consistent for elements, thanks!

[Ruslan-Aleev]

I accidentally noticed that when creating a resource from the menu - "undefined" is indicated in the resource header, although there is no such problem in the current version of MODX 3.

[smg6511]

@Ruslan-Aleev - Good catch! This is fixed with the latest commit. Quick question to you and others: I noticed that the term shown in the panel title had been changed to "Create Document" at some point, so I matched that ... wanted to make sure that's correct. If so, that change in terminology from "Resource" to "Document" should be carried through in other places (such as the tree's main menu and in the panel tabs); that would be for another PR if that's the intended term to be used moving forward.

[JoshuaLuckers]

Quick question to you and others: I noticed that the term shown in the panel title had been changed to "Create Document" at some point, so I matched that ... wanted to make sure that's correct. If so, that change in terminology from "Resource" to "Document" should be carried through in other places (such as the tree's main menu and in the panel tabs); that would be for another PR if that's the intended term to be used moving forward.

A "Resource" (modResource) needs/has a "Resource Type" e.g Document, Weblink, Collection, Article etc. The default resource type is "Document".

[smg6511]

@JoshuaLuckers - OK, I see. So then, to be completely correct, if one is creating a new Resource and changes the type to something other than the default (Document) the panel title should reflect that and the first tab should as well (e.g., if you choose Symlink, the initial panel title should read "Create Symlink" and the first tab should change to "Symlink." I'm not suggesting this detail be addressed in this PR, but maybe it should be handled in a separate one at some point.

[Ruslan-Aleev]

@smg6511 After last edit, I get a 500 error:

PHP Fatal error:  Declaration of MODX\Revolution\Services\Container::get(string $id) must be compatible with Psr\Container\ContainerInterface::get($id) in /core/src/Revolution/Services/Container.php on line 8

[smg6511]

@Ruslan-Aleev - Yeah, I had that too when I switched branches. You have to run composer update. This is related to a change recently made in XPDO.

[smg6511]

@opengeek @Mark-H - Now that 3.0 is out and we're in the 3.0.1 dev cycle, can we please merge this so I can put up the related PRs for review?

[opengeek]

Just to confirm, this PR does not introduce any features or compatibility issues for CMPs or other extras that might target 3.0.0, correct?

[smg6511]

No, I don't believe the changes in this and the forthcoming PRs should have any effects on extras/CMPs.