Skip to content

Prevent empty label divs#450

Open
hughbris wants to merge 7 commits intogetgrav:developfrom
hughbris:prevent-empty-label-divs
Open

Prevent empty label divs#450
hughbris wants to merge 7 commits intogetgrav:developfrom
hughbris:prevent-empty-label-divs

Conversation

@hughbris
Copy link
Copy Markdown
Contributor

I noticed the logic for showing labels using show_label isn't implemented as early as it should be.

This means when labels aren't being shown, you still get the label wrapper divs rendered, causing problems like asterisks for required fields attached to nothing (empty label divs). Try it with label false and required true on a field.

@rhukster
Copy link
Copy Markdown
Member

rhukster commented Dec 1, 2020

I'm seeing is you've moved the label block outside the show_label check. That doesn't change how the internal elements are rendered.

@hughbris
Copy link
Copy Markdown
Contributor Author

hughbris commented Dec 2, 2020

It allows overriding the label block so the "ghost" label won't render anyway. This is what I have done. (Also I only noticed it because of the red asterisk on it.) I must apologise because I can't remember more about my motivation, however I did find this necessary about 1.5 weeks ago. Too much water under the bridge :) If I remember more, I will add here.

It does seem logical to me that everything label-related is within the label block and I presume it's a harmless mod.

@rhukster
Copy link
Copy Markdown
Member

rhukster commented Dec 3, 2020

I need a bit more time to test this especially in regard to the other PR that removes label block entirely from the checkbox field: #442

@hughbris
Copy link
Copy Markdown
Contributor Author

hughbris commented Dec 3, 2020

Thanks, understand. My early impression of the other PR is that it wouldn't be needed if this goes in, and this has less likely unintended consequences.

<label class="{{ layout_form_field_label_classes }}{{ form_field_label_trim }}" {% if field.id %}for="{{ form_field_for }}"{% endif %}>
{%- block label -%}
{%- if form_field_help -%}
<span class="tooltip" data-tooltip="{{ form_field_help|e }}">{{ form_field_label|raw }}</span>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the escaping removed here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like us using |raw without concent from the person who writes the form. Either the property should be called label_html or it should indicate it in some other way, maybe something like

#474 (review)

but with html: true in it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in this case it is a attribute, so it MUST use escape anyways. It should be added back anyways.

What about this PR itself? Will it be merged?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants