Skip to content

[Port to 3.x] Fix Various Issues with Form Panel Validation and Field Display - Part 5, Chunk Panel #16128

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

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Apr 7, 2022

NOTE: This is part 5 of 6 of a revised submission of #15887. This should be reviewed after part 2 is merged (there are two small changes needed from that PR to make the locked switch appear correctly).

What does it do?

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

  1. Updated panel JS to make form layout consistent with the layout introduced in the current TV panel, and utilize new shared methods introduced in the main FormPanel via part 1 of this PR set (already merged).
  2. A handful of minor lexicon updates.

Why is it needed?

Makes needed UI and consistency improvements to the Chunk 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 and update a few Chunks to verify behavior and appearance is as expected.

Related issue(s)/PR(s)

Partial port of #15146

@smg6511 smg6511 requested review from opengeek and Mark-H as code owners April 7, 2022 02:12
@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Apr 7, 2022
@smg6511 smg6511 added the pr/review-needed Pull request requires review and testing. label Apr 7, 2022
@smg6511 smg6511 added this to the v3.0.1 milestone Apr 7, 2022
Jim Graham added 3 commits April 16, 2022 23:42
Tweak to prevent undefined error when keyup is fired on empty field
@smg6511 smg6511 force-pushed the 3.x-port-pr-15146-rev-part-5 branch from 45b0728 to d77df24 Compare April 17, 2022 03:42
@theboxer
Copy link
Member

@smg6511 If the chunk name is short enough, this happens :)

2022-04-20 16 11 49

@smg6511
Copy link
Collaborator Author

smg6511 commented Apr 20, 2022

@smg6511 If the chunk name is short enough, this happens :)

It's not the length of the element name, it's where it happened to end up in the description that's causing that to happen. What type of device are you viewing on in this example?

@theboxer
Copy link
Member

Chrome, macbook, external 24" screen. And it's the length of the element, if you hover the tagname to copy it, it appends the copy icon. If the element name is long enough, the icon will push the whole tag to the next line and as you stop hovering it, the icon disappears.

@smg6511
Copy link
Collaborator Author

smg6511 commented Apr 20, 2022

It's ultimately the description's line length though, it has to be just right for the example tag to fall right at the end. I debated making the fool-proof move of returning the tag to a new line (in which case what you pointed out would never happen), but it's such a waste of space to cover a scenario that would so rarely occur. The only other way would be to not offer the copy icon visual cue on hover. I thought it was important to give that hint though.

I suppose another way around it would be to give the hint via a popover (QuickTip) instead of the icon.

@theboxer
Copy link
Member

@smg6511 tbh. I don't think this is a blocker for merging these PRs. Just wanted to point out that this is happening in some specific cases. I'd vote for merging and maybe finding a reasonable solution for next version?

@opengeek opengeek merged commit 5bc2297 into modxcms:3.x Apr 20, 2022
@smg6511 smg6511 deleted the 3.x-port-pr-15146-rev-part-5 branch July 19, 2022 14:53
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