-
-
Notifications
You must be signed in to change notification settings - Fork 529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3.x] Refine Quick Create/Edit Windows for Elements #15953
base: 3.x
Are you sure you want to change the base?
Conversation
@smg6511 I do like these changes, except setting the window mode to |
You can call 2 windows, or work simultaneously with the edit form and the quick window (for example, when I call the snippet, I write the names of the chunks, and in parallel, I create these chunks through QCE), this will not work with a modal window.
Disagree, the interface is now more inconsistent.
And the global question: are quick windows so necessary in changes that you touch as many as 1000 lines of code? I am 100% sure that if the PR is merged, then in the future we will catch bugs. |
Ok, fair enough. I knew the use of modal may be questionable for some. I don't think I've ever thought about opening multiple QCEs when working in a site. This is where it's important to get the reaction of others who have different work styles ;-) . Perhaps a more elegant interface for these quick editors could be conceived in the future. I'll play with trying to create separation in a different way.
Yes, right at this moment. I would carry through whatever changes get accepted here to all windows as well. I am trying to appease the request to have smaller, more easily mergeable PRs by not doing it all in one swoop here. On the whole, the descriptions of fields should show if
I’m not suggesting we change the help setting options in this PR, just that it’s a way that makes more sense to me, and could be considered for future implementation.
You should see them for all element windows in all but the top row of fields (which I debated adding; I probably should for full consistency). At any rate, I had a lot of trouble with the MODx cache not fully clearing when I was making and reviewing these changes. Sometimes I had to navigate away from where I was and clear the cache more than once. Lastly, the concern about touching many lines of code and bugs is less than you might think with this IMO. I'm not touching any code affecting how these windows process their data, it's all presentational. And, no window JS classes extend these already extending (MODx.Window) classes (they're the final in the chain). |
No offense, but your PR looks like this to me:
There are a lot of objective problems in MODX, maybe it's worth fixing them? I'm not trying to tell you what to do, but there are few active participants in the MODX repository and I feel sorry for your efforts and time. |
@Ruslan-Aleev - A couple important things re your comment: I think you misunderstood my previous reply (or I wasn't completely clear) —
On the lexicons issue, I understand that there's not an army of people working on MODx. But shouldn't we be striving for excellence and completeness in the interface? I know you’ve been trying to streamline the lex's to make it easier to maintain, but I think prioritizing that makes for an interface that lacks the polish it could and, IMO, should have. To me, ahead of this 3.x release is the perfect time to try to hone the basics with the lex's and in other areas; make the keying and naming conventions as consistent as possible, make the labelling, descriptions, and other messaging more specific to their contexts. Using say, a date TV as an example: “Default Date” is just more friendly and communicates immediately, as opposed to “Default Value” which is more programmer speak than natural language. Yes you can figure out what should go in that field, but honestly, which is better? Finally, don't get me wrong — I am an avid proponent of MODx and have been for years — but the more I look, the more I see a significant number of problems that deserve attention (regardless of whether or not they are highly visible or have made it to the issues list). And many of my PRs set out to solve an issue in the list, but balloon into much more because other issues become apparent along the way. Again, I get that this is an open source project with limited resources. But glossing over details too much to push things ahead should be avoided. |
Thanks for all the work you've put into this @smg6511 ! Personally I'm fine with the changes... except for a few points. 😉
|
@muzzwood - Thanks for your feedback! Just to explain the logic of why I chose to lay out the TV window in the way you see (not to say I won't change it if folks don't like it):
FYI, both the Default Value and Input Options fields are textareas, they're just initially one line in height to conserve space. They're both set to grow vertically to a defined maximum. On the help text, there's some brainstorming to be done there and a more consistent and elegant solution to be had. I've thought of something somewhat similar to what you've suggested, but it's a feature best worked out in a different proposal/PR. A couple things for all to note:
|
We need a sort of a click through layer (CSS: |
@Jako - Thanks for the hint! I took a quick look and should be able to work something out using the strategy you mentioned. |
@smg6511 I hope a modal background is just added once in the DOM and there are no other disabling strategies executed in ExtJS when |
@Jako - Yes, I've already sorted out how to do it with just one overlay; it doesn't use the modal mode, as it would've been more complicated to override the associated methods in the Window class and we want to retain the established modal behavior in place for dialogs and message windows. |
Here's a quick recording of the custom 'modal' feature (I'm calling these pseudo modals) in action. Thanks again @Jako for the pointer-events suggestion (of which I wasn't aware of since I've not come across the need for it until now). modx-qce-pseudoModals-ex3-720.mov |
@smg6511 And someone else besides you is confused by the style of the quick window? Maybe you should create a poll in the community https://community.modx.com/? |
Visually love this a lot. From an accessibility perspective, I think it bears a small "Remove mask" button so everyone can more easily read the contents of the left tree by removing the grey background overlay. Awesome job! |
A 'remove mask' is also an option for the 'modal' background. Nice idea! |
This looks fantastic @smg6511 ! |
@smg6511 I'm not sure if showing the darker overlay while allowing the interactions on the background is a good idea. You'll have same UI for modals where you can interact with background objects and for modals where you can't - which might confuse people that are used to not beein able to interact with background objects when the dark overlay is present. So until you'll try, you won't know if you can or can't do stuff in background. I'd vote for keeping the dark overlay only for modals where you can't do any interactions with background objects. |
@theboxer - I take your point about the overlay color (if they are the same for all modal-type windows). A couple of things to note:
The above said, and with other feedback taken into account (such as adding the ability to disable the overlays, or otherwise control them), I think we could arrive at an overall better UI with the windows that everyone could be happy with. @rthrash - Do you think it'd be more useful to have the ability to turn off the overlays at the page/window level or should we simply add a system setting where you globally turn them on/off. Also, we could make modal overlay opacity user-customizable in a system setting ... would that be desirable? @Jako - Given my what I said above (real modals vs. others), I don't think it'd make sense to be able to remove a blocking (real) modal's overlay as there's no reason (or ability for that matter) to interact with the interface below. |
@smg6511 how it will work for windows that are using the |
Nothing changes for windows set to |
👍 |
It should be a global setting for sure, with an opacity setting as a nice to have. I would be nice to be able to have a button on pseudo-modals for disabling on pages where they're used, but I'm probably a 5/10 on that front. |
Guys, I wanted to pass this proposed layout change by you for windows having the "Empty Cache" switch; see below. I've placed the switch in the window’s footer bar to gain a bit more space and neaten the form a bit. Also, now that I realize that this cache setting is not persistent (not part of any db record) and is applied on a save-by-save basis, it makes more logical sense for it to be grouped with the action buttons. An aside on the Clear Cache option: I'm not proposing a change here at this time, but it occurs to me that it'd make more sense (at least to me) for the switch to be "Preserve Cache on Save" with it being off by default. This results in effectively the same default behavior, just advertised to the user differently. Curious what you all think. Also, a technical detail I'd like opinion on: Unless there's a known special reason for it, IMO the inline help components should be of xtype
to:
Lastly, I'm wondering for the TV QCE window whether it'd be better to provide a tabbed form like the QCE Resource window does. Do you all tend to set up input options and default values a lot when working in these windows? That's it for now. I'll be working on a couple of the other requests in this conversation before pushing new changes. |
a79b8a2
to
45d2bab
Compare
I never use the quick create TV option. |
648b19a
to
ec33674
Compare
@smg6511 Hey Jim you want to revisit this for 3.1? Was a very nice net positive that sounds like it was lacking a bit of finalization. |
@rthrash - Yeah, I was just looking at this a couple days ago. I'll pick it back up in the next couple weeks or so. I shifted focus away from this, knowing that it was going to take some time to get it through, to knock out some of the many issues we have. |
Completely understand @smg6511. We're trying to get a release out soon, so maybe this should be pushed to a later release. I would love to have this one in there, though, as it's such a nice UX improvement! |
How soon are you targeting the release? I'll try to squeeze it in ... |
Before July |
Is this only a UX improvement, I'm thinking it could potentially go into a patch release. If that is the case, there is no need to rush it. |
1933ff4
to
af7ed86
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #15953 +/- ##
============================================
+ Coverage 21.62% 21.73% +0.11%
+ Complexity 10588 10483 -105
============================================
Files 561 561
Lines 32033 31620 -413
============================================
- Hits 6926 6873 -53
+ Misses 25107 24747 -360 ☔ View full report in Codecov by Sentry. |
OK, I'm almost there folks ... been working on this over the past couple weeks. I have a few UI things to button up and the actual persistence routine to implement, but here's a quick video illustrating a new mask control feature that I think will seal the deal with this PR's proposed overall change to window masking (which, in my local WIP, applies to all standard-built* MODX windows, not just the handful handled in the current iteration of this PR).
Screen.Recording.2024-07-19.at.11.30.31.AM.mov |
Well done! |
I'm making my way on this, slower than expected ... the more testing, the more little things I find need fixing to make it all work properly. In the meantime, here's a teaser of a fix that makes the window UI overall more consistent: Now the true modal/dialog windows animate the same as all others. It took a bit of doing/reverse engineering and sifting through the Ext source code, but solvable it was! Screen.Recording.2024-07-30.at.11.23.43.AM.mov |
Post-rebase fixes and and updated modal behavior in response to reviewer feedback.
af7ed86
to
b54df67
Compare
Adds replacement method for getting usergroup ids, allowing additional params to be passed. Reason for change is to support access control of window settings (made available in a new config window) that honors usergroup permissions.
Now that user changes to color and opacity are interactive via new config window, no need for this library (for calculating color variants)
Interim progress, tweaks
Code QC
@smg6511 Hey Jim, I'm revisiting these changes and I like the overall quick window changes for the input placements, but I have few things:
|
@theboxer Jan - yeah, I took a break from working on in a little while ago but will come back to it. There's a lot I've done that you don't see yet which I think would satisfy most of the things you don't like (particularly the interactive control of the mask appearance as shown in the screen video above). I might go ahead and change this to a draft to make it clear it's not yet ready for a final review. One thing I might disagree with you on is the animation of overlays; IMO a sudden appearance of such elements feels unpolished. I'll give you this though—I might have them running too long now (that can be played with obviously). And, if it were anything but a simple fade transition I'd totally agree with you on this front. |
What does it do?
Made changes to improve the layout of the quick create/edit TV window:
Below is a screenshot of how the window should appear:
Why is it needed?
How to Test
grunt build
from within the_build/templates/default
folder and clear your browser cache.Special Note
Ultimately, for the TV QCE window, more work will need to be done in a separate PR to bring the tv type-specific field display of the main TV panel to the associated QCE window (for the default value and input options fields). Also, I am aware that changing the window to a modal prevents the ability to drag/drop tree items into the window's fields. IMO, that's not particularly useful in the QCE windows and is problematic from my testing; it's very easy to unintentionally drop an item in the form beneath without knowing it. I've also found it very distracting to have the front window and the main page visually on the same plane.