Skip to content
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] & [4.x]: Modified Attributes status-badge works oddly with compound fields #12403

Closed
khalwat opened this issue Nov 30, 2022 · 5 comments
Closed
Assignees

Comments

@khalwat
Copy link
Contributor

khalwat commented Nov 30, 2022

What happened?

Description

I have several plugins that use "compound fields", meaning that they provide field types that put more than one field of data for the user to enter in a single Craft CMS field type: SEOmatic / Recipe / Code Field (there may be more as well)

The problem is Craft's relatively new Modified Attributes setup uses JavaScript to add <div class="status-badge"> injected into the field HTML, but it doesn't seem to work well with field types that have more than one field of data in them. These fields are added via the Craft forms macros, so they and up with wrapper divs around them like <div class="field">

I also surround these fields with <fieldset> for semantic HTML / a11y reasons. Here's the result:

Craft 3:

Screen Shot 2022-11-30 at 12 16 39 PM

Screen Shot 2022-11-30 at 12 17 30 PM

On Craft 3, even though I'm changing a field several fields down, it adds a <div class="status-badge changed"> to both the surrounding field, and also to the first <field></field> in the compound field list

Also note that in SEOmatic's case, there's an additional <div class="field"> added (look closely and you can see two blue lines next to each other near the title SEO). I think this is because I wrap the entire SEOmatic field in <div class="field">, and then use <fieldset> around each set of fields in each tag... which maybe I shouldn't be doing? Unclear, but the problem exists with the additional line on the first inner <div class="field"> anyway.

Craft 4:

Screen Shot 2022-11-30 at 12 19 01 PM

Screen Shot 2022-11-30 at 12 19 15 PM

On Craft 4, it does the same thing, but presumably, due to CSS changes, it looks even worse.

Also note that in SEOmatic's case, there's an additional <div class="field"> added. I think this is because I wrap the entire SEOmatic field in <div class="field">, and then use <fieldset> around each set of fields in each tag... which maybe I shouldn't be doing? Unclear, but the problem exists with the additional line on the first inner <div class="field"> anyway.

This a done via this chunk of code in ElementEditor.js:

            // Add missing field modified indicators
            const selectors = response.data.modifiedAttributes
              .map((attr) => {
                attr = this.namespaceInputName(attr);
                return `[name="${attr}"],[name^="${attr}["]`;
              })
              .concat(modifiedFieldNames.map((name) => `[name="${name}"]`));

            const $fields = $(selectors.join(','))
              .parents()
              .filter('.field:not(:has(> .status-badge))');
            for (let i = 0; i < $fields.length; i++) {
              $fields.eq(i).prepend(
                $('<div/>', {
                  class: 'status-badge modified',
                  title: Craft.t('app', 'This field has been modified.'),
                }).append(
                  $('<span/>', {
                    class: 'visually-hidden',
                    html: Craft.t('app', 'This field has been modified.'),
                  })
                )
              );
            }

For now, I have worked around it with some CSS roughly like this:

div.inheritable-field .status-badge {
    display: none;
}

...but it feels a little gross.

Steps to reproduce

  1. Install SEOmatic, Recipe, Code Field, or any plugin that has "compound fields"
  2. Create the appropriate field type
  3. Add it to your Section layout
  4. Edit the entry, and make a change to one of the fields in the middle of the compound field

Expected behavior

Since currently, the system can't know which field in a compound field has changed, it should probably only mark the parent field itself with the <div class="status-badge changed">

Alternatively, instead of looking for just field perhaps it could look for fieldset too, if if that is present, put the badge only there, but not in child field elements. This would require that everyone using compound field types wrap things in <fieldset> but since that's probably the best approach anyway, maybe it would encourage best practices.

Actual behavior

It marks both the field itself, and also the first child field (regardless of which part of the form changed) as changed. And the CSS on Craft 4 makes it look not good from a padding perspective.

Craft CMS version

3.x & 4.x

PHP version

n/a

Operating system and version

n/a

Database type and version

n/a

Image driver and version

n/a

Installed plugins and versions

  • SEOmatic
  • Recipe
  • Code Field
@brianjhanson brianjhanson self-assigned this Nov 30, 2022
brianjhanson added a commit that referenced this issue Nov 30, 2022
Udpates the status indicator to hook onto the `data-layout-element` attribute vs. the `.field` class because the `.field` class might be used in other contexts.

Fixes #12403
@brianjhanson
Copy link
Contributor

Thanks for reporting! I just opened a few PRs to address this. Instead of just looking for .field classes, we'll now look for first level children under a .flex-fields selector (.flex-fields > .field).

The bummer is we'll lose some of the granularity you get on fields that have been edited, but it should be relatively simple to get back if desired. You'd just need to wrap .field classes with a .flex-fields class.

@khalwat
Copy link
Contributor Author

khalwat commented Nov 30, 2022

Well, it never worked to begin with for the changed to sub-fields in a compound field type (it always just highlighted the first field). There's no way that I know of it indicate which sub-fields have changed for a Field Type? If I'm missing something, I'd love to know!

@brianjhanson
Copy link
Contributor

Nope, you're right. I saw the first field highlighted and got confused. It looks like we intentionally only grab the first param over here. I believe that was done primarily for performance reasons.

khalwat added a commit to nystudio107/craft-code-field that referenced this issue Nov 30, 2022
khalwat added a commit to nystudio107/craft-code-field that referenced this issue Nov 30, 2022
khalwat added a commit to nystudio107/craft-recipe that referenced this issue Dec 1, 2022
khalwat added a commit to nystudio107/craft-recipe that referenced this issue Dec 1, 2022
@brandonkelly
Copy link
Member

This is fixed for the next Craft 3 and 4 releases, via #12405.

@brandonkelly
Copy link
Member

Craft 3.7.64 and 4.3.7 have been released with that fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants