Skip to content

[Port to 3.x] Fix Various Issues with Resource/Element Validation and Field Display #15887

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

Closed
wants to merge 4 commits into from

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Nov 4, 2021

What does it do?

Ports enhancements from PR #15146 to 3.x (including subsequent fixes by others) and incorporates UI updates introduced in PR #15773 made to the TV editing form into the rest of the element editing forms. Other changes include:

  1. Consolidated repetitive method definitions found in the element widget classes, placing single replacement methods into the parent FormPanel class.
  2. Addressed two concerns raised by @Ruslan-Aleev in UX inaccuracies in TV creation forms #15854 : (1) Fine-tuned style for tag copy to make clear that the icon that shows on hover is only a hint and cannot be clicked to perform the copy, and (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. Made language changes to the element tab introduction text for clarity, including an anchor link to the element code (when applicable)

Why is it needed?

Makes needed UI and consistency improvements to the resource and element editing panels.

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 a handful of 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.
  4. Create at least one test element for each type (chunk, plugin, template, tv), and verify each appear, behave, and save as expected.

Related issue(s)/PR(s)

Partly addresses #15854.

Jim Graham added 2 commits November 4, 2021 13:31
Port of PR#15146 to 3.0 as well as many form panel code optimizations
...and one bug fix in resource panel
@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Nov 4, 2021
@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 4, 2021

One important thing I wanted to note and get opinion on: As you all know the template element has a new preview feature. This is great, however it introduced an issue I didn't fully address in this PR: Now there are two file type fields in the editing form that need to refer to a Media Source, but sharing that source becomes a UI issue because of how the static file/source fields are now grouped together and hidden when not is use. The solution that comes to mind is to add a separate source field to associate with the preview file, an option that may be desirable by some anyway.

@opengeek opengeek added the pr/review-needed Pull request requires review and testing. label Nov 5, 2021
@smg6511
Copy link
Collaborator Author

smg6511 commented Dec 8, 2021

@Mark-H @Ruslan-Aleev - Just bumping this, in particular to those who tested and approved these changes that were merged into 2.x (about this time last year). It'd be great to get this in before final release ;-)

@Mark-H Mark-H added this to the v3.0.0-rc2 milestone Jan 19, 2022
@opengeek
Copy link
Member

Why are there merges into this PR from 3.x? In the future, please rebase your work on top of latest 3.x and force push instead.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 26, 2022

@opengeek - Sorry, you're witnessing some of my lack of experience in the GIT world. So I'm guessing rebasing is the strategy for keeping an in-progress branch in sync with the branch it was based from; and I take it that keeps you from seeing all the un directly-related commits? Would it be of any help to rebase this branch now?

@opengeek
Copy link
Member

@opengeek - Sorry, you're witnessing some of my lack of experience in the GIT world. So I'm guessing rebasing is the strategy for keeping an in-progress branch in sync with the branch it was based from; and I take it that keeps you from seeing all the un directly-related commits? Would it be of any help to rebase this branch now?

No reason to rebase now. Merge commits are just messy. I'm of the philosophy that you should always rebase unless it is on a branch that is meant for public consumption or mass collaboration and is already published. At that point, merging is the only option.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 28, 2022

@opengeek @Mark-H @Ruslan-Aleev @Jako @Ibochkarev - OK all, as I'm having trouble getting this looked at as a whole (and I inconveniently made a couple 3.x merge commits), I'm going to break this up into a handful of separate PRs. Note that I'm submitting the first now (focused on the Resource panel), which should supersede the change @Ruslan-Aleev submitted in PR #16008. Again, to remind, this PR as a whole (minus a couple small updates) had already been merged into 2.8.0 back in Oct 2020, which is why I thought it would be relatively easy to get it merged into 3.x.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 29, 2022

I'm pulling this PR down and am implementing these changes in a revised set of PRs beginning with #16012 in an effort to make the changes more easily (and hopefully quickly) reviewed. Once #16012 is merged, I'll submit the other parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants