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

[com_fields] Adding label Class for input #18431

Merged
merged 10 commits into from Jun 29, 2018

Conversation

Projects
None yet
9 participants
@coolcat-creations
Contributor

coolcat-creations commented Oct 27, 2017

Pull Request for Issue #16994.

Summary of Changes

Removed the hide Label Option
Replaced hide Label Option with a label Class Option.
Added B/C Option to add class hidden to the class if label was set previously to hidden.
Added a label class Option for Form
Reordered the Options and added headlines.
See also discussion in #17177

Testing Instructions

1.	Hide your label, apply the patch and see if the label is still hidden
2.	Add a class for the label in the form and see if it's added
3.	Add a class for the label in the output rendering and see if it's added

Documentation Changes Required

Yes adding the possibility to add a Label Class in the form.

grafik

grafik

@ciar4n

This comment has been minimized.

Member

ciar4n commented Oct 28, 2017

A slight B/C issue. If the 'Show Label' has been set to hide, once the PR is applied there is no way to un-hide the label. The hidden class is saved to the user data with no way to remove it.

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Oct 28, 2017

@ciar4n good point. Any other idea how to make it B/C able?
hmmm...

@ciar4n

This comment has been minimized.

Member

ciar4n commented Oct 29, 2017

In this case none that I can think of.

I think for now we could keep the 'Show Label' fields. Considering the 'Render Class' exists which is adding a class to the field container, I would question the need for a 'Label Class'. So my 2 cents would be to revert replacing the 'Show Label' fields with the 'Label Class' fields.

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Oct 29, 2017

Yes, you are right, we can control the label by the render class. Just tried to be more consistent for both places. Will change it.

coolcat-creations added some commits Oct 29, 2017

Add back showlabel for the render options
Remove label class for render options
Moved field to the right position
Language String changed
@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Oct 29, 2017

Looks like that now:
grafik

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 2, 2017

This works fine, but I really would like to get a Label Class when rendering.
What we have now is limited to all fields field-label

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 2, 2017

@infograf768 not sure what you mean? Can you try to explain it different, what the issue is?

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 2, 2017

In this patch, the Label Class is used when editing an item in backend.
Example adding class invalid to label:
screen shot 2017-11-02 at 17 15 24

gives when editing an article in backend:
screen shot 2017-11-02 at 17 16 24

But this can't have any effect on frontend
screen shot 2017-11-02 at 17 17 52

To be able to render a specific class in frontend, you need a new field in the xml, and then modify the files
ROOT/components/com_contact/layouts/field/render.php
and
/ROOT/components/com_fields/layouts/field/render.php

to add that class after field-label

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 2, 2017

That new field could be displayed with a showon depending on the value of the Show Label field

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 2, 2017

@infograf768 i still don't understand. The label class is only for the form (input) not for the rendering part. If you look at the options I even added headlines to clarify this.

In the first version of the PR i did quite exactly what you are suggesting but we said then to remove the label class for frontend again and to leave the show / hide field for rendering.

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

I still think it is quite helpful to be able to add a specific class in frontend for the label rendering when rendering Label is set to Yes. This will let using different rendering depending on the field.

The B/C issue pointed at by @ciar4n does not exist as far as I could test.
A field formerly set to Hide Label let's change it to show after my patch.

When hiding label, the render label class is just not used as the label is hidden.

This is what I have done:
Added a new field in in /administrator/components/com_fields/models/forms/field.xml (the lang strings are already in your PR) after Show Label:

			<field
				name="render_label_class"
				type="textarea"
				label="COM_FIELDS_FIELD_LABEL_RENDER_CLASS_LABEL"
				description="COM_FIELDS_FIELD_LABEL_RENDER_CLASS_DESC"
				class="input-xxlarge"
				size="40"
				showon="showlabel:1"
			/>

Added the node in fieldsplugin.php line 174 (not sure that one is necessary)
$node->setAttribute('renderlabelclass', $field->params->get('render_label_class'));

Modified /components/com_fields/layouts/field/render.php to be able to use the render_label_class

if (!key_exists('field', $displayData))
{
	return;
}

$field = $displayData['field'];
$label = JText::_($field->label);
$value = $field->value;
$showLabel = $field->params->get('showlabel');
$renderLabelClass = $field->params->get('render_label_class');

if ($value == '')
{
	return;
}

?>
<?php if ($showLabel == 1) : ?>
	<span class="field-label <?php echo $renderLabelClass; ?>"><?php echo htmlentities($label, ENT_QUOTES | ENT_IGNORE, 'UTF-8'); ?>: </span>
<?php endif; ?>
<span class="field-value"><?php echo $value; ?></span>

Same has to be done for the specific /components/com_contact/layouts/field/render.php

I get:
customfieldslabelclass

EDIT: if I un-hide the Label, it works fine.

@ciar4n

This comment has been minimized.

Member

ciar4n commented Nov 3, 2017

The B/C issue has been fixed with 741c13d

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

With my patch, as explained above, I could not reproduce your B/C issue, which means we can add the render_label_class as suggested.

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

If you like, I can show you with animated screenshot

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

@ciar4n
Shall I do these captures?

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 3, 2017

@infograf768 you can define the class of the label by .renderclass label thats why we dont add another additional parameter. We start adding too much parameters again. This PR is just to be able to add a class for the label in the form in order to hide it or to do different things. it would be better of course to have the same for the Rendering too but there is already a show/hide field. Means we would need to add another field. As far i know we wanted to prevent to add too many options?

@ciar4n

This comment has been minimized.

Member

ciar4n commented Nov 3, 2017

My apologies.. I thought your b/c comment was on the PR in general rather than your specific suggestion. As your suggestion doesn't remove the 'Show Label' field, I see no b/c issue with it.

Adding a 'Render Label Class' certainly makes things a little easier for alternative styling on the label. My only thought on it is that in the case of the render field, you could use the 'Render Class' field to do the same thing. The difference been that you will need to create custom CSS for the class rather than been able to use a currently existing bootstrap class.

.my-label-class label {
   // New label styling
}
@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 3, 2017

or
.my-containerclass label + label {
}

it's just not a label in the rendering, i think it's a span

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

In fact, it would rather be

.field-label .my-label-class {
    // style
}

to target specifically the custom field markup as we would have
<span class="field-label <?php echo $renderLabelClass; ?>"

In any case, I really think we need this even if it is a new parameter.
@coolcat-creations
Would you accept to update your PR?

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 3, 2017

I can add back the class of course. that was initially what i wanted to do. Just did not want to overcrowd the parameters again.

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 3, 2017

Thanks. Will test when done.

@coolcat-creations

This comment has been minimized.

Contributor

coolcat-creations commented Nov 3, 2017

Done :)

@@ -298,6 +298,16 @@
</field>
<field
name="label_render_class"

This comment has been minimized.

@infograf768

infograf768 Nov 3, 2017

Member

1 tab too much for the new field name etc..

@infograf768

This comment has been minimized.

Member

infograf768 commented Nov 13, 2017

I have tested this item successfully on 7351e21


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18431.

@infograf768 infograf768 added this to the Joomla 3.8.3 milestone Nov 13, 2017

@@ -31,6 +33,10 @@ COM_FIELDS_FIELD_IMAGE_LABEL="Image"
COM_FIELDS_FIELD_INVALID_DEFAULT_VALUE="The default value is invalid."
COM_FIELDS_FIELD_LABEL_DESC="The label of the field to display."
COM_FIELDS_FIELD_LABEL_LABEL="Label"
COM_FIELDS_FIELD_LABEL_FORM_CLASS_LABEL="Label Class"

This comment has been minimized.

@Quy

Quy Nov 15, 2017

Contributor

Alphabetize keys from lines 35-39.

This comment has been minimized.

@infograf768

infograf768 Nov 15, 2017

Member

oops, did not check that one. Good find

This comment has been minimized.

@coolcat-creations

coolcat-creations Nov 18, 2017

Contributor

Thank you, learned something new :)

@Joomill

This comment has been minimized.

Joomill commented Dec 10, 2017

I have tested this item successfully on 9721f9f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18431.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.9.0 milestone Dec 10, 2017

@franz-wohlkoenig

This comment has been minimized.

franz-wohlkoenig commented Dec 10, 2017

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Dec 10, 2017

@infograf768 infograf768 added this to the Joomla 3.8.4 milestone Dec 11, 2017

@mbabker mbabker modified the milestones: Joomla 3.8.4, Joomla 3.9.0 Dec 21, 2017

@mbabker mbabker changed the base branch from staging to 3.9-dev Jun 2, 2018

@mbabker mbabker modified the milestones: Joomla 3.10.0, Joomla 3.9.0 Jun 2, 2018

@mbabker mbabker added PR-3.9-dev and removed PR-staging labels Jun 2, 2018

@mbabker mbabker requested a review from brianteeman as a code owner Jun 2, 2018

@mbabker mbabker requested a review from laoneo Jun 2, 2018

@mbabker mbabker merged commit 4c1fea1 into joomla:3.9-dev Jun 29, 2018

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 29, 2018

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