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

Fix form-horizontal for non-latin or long language strings #15689

Merged
merged 8 commits into from May 22, 2017
Merged

Fix form-horizontal for non-latin or long language strings #15689

merged 8 commits into from May 22, 2017

Conversation

DenysNosov
Copy link
Contributor

@DenysNosov DenysNosov commented Apr 30, 2017

Pull Request for Issue # .

Summary of Changes

Fix form-horizontal for non-latin or long language strings.

Please look for this languages specifies fix: @infograf768

Before fix:
before

After fix:
screenshot-lang j-2017-04-30-04-20-12

Optional width 200px:

max-width: 200px;
min-width: 200px;
width: 200px;

Testing latest version Firefox, Chrome and etc Chromius based brawsers.

@CB9TOIIIA
Copy link

CB9TOIIIA commented Apr 30, 2017

Joomla pain :)

@b2z
Copy link
Member

b2z commented Apr 30, 2017

As a Russian language user I am definitely for it. Looking much better now.

@Arkadiy-Sedelnikov
Copy link
Contributor

Arkadiy-Sedelnikov commented Apr 30, 2017

Agree

@ghost
Copy link

ghost commented Apr 30, 2017

if 2 People test this PR successfully it could go in staging if Maintainers agree.

@ot2sen
Copy link
Contributor

ot2sen commented Apr 30, 2017

I have tested this item successfully on cc318d8

Tested with multiple languages and Russian in particular which had this issue.
Logic improvement on i18n to ensure any language looks good/correct in the interface for admin forms.


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

@brianteeman
Copy link
Member

brianteeman commented Apr 30, 2017

Please do not edit the css file directly. That is not correct.

The correct way is to make the changes in the less file and then run generatecss.php from the build directory to create the css

@infograf768
Copy link
Member

infograf768 commented May 1, 2017

Unhappily, we have multiple .form-horizontal .control-label in the less => css files.
Some have width: auto; depending on the screen width.

Therefore the less files to modify has to be chosen carefully
Also nothing is changed for rtl here.

@infograf768
Copy link
Member

infograf768 commented May 1, 2017

@ciar4n
Can you have a look?

@ciar4n
Copy link
Member

ciar4n commented May 1, 2017

The Bootstrap default is for the control-label to wrap at 160px so if the intent is to allow the label to wrap then simply removing https://github.com/joomla/joomla-cms/blob/staging/administrator/templates/isis/less/blocks/_forms.less#L10 should give the same result. If you wish to increase the label width you will also need to increase the left margin on .form-horizontal .controls by the same amount. Not sure if there is a need for setting the min and max width.. maybe you have your reasons?

Test RTL however it should be ok. Media queries should also be fine as is.. smaller screen sizes have the label and control in columns.

As mentioned by @brianteeman you will need to edit the LESS files and then compile rather than editing the CSS directly (https://docs.joomla.org/Joomla_LESS).

Fedik and others added 2 commits May 3, 2017
Limit a control-label width in form-horizontal
@Fedik
Copy link
Member

Fedik commented May 3, 2017

I have tested this item successfully on 6250e2f


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

@Quy
Copy link

Quy commented May 3, 2017

It looks good. The only 1 display issue is under System > Global Configuration > Media that just needs markup change.

Before:
media-before

After:
media-after

@brianteeman
Copy link
Member

brianteeman commented May 3, 2017

@Quy that might happen in other places too especially with extensions

@Fedik
Copy link
Member

Fedik commented May 3, 2017

it because a Spacer field displaying the text as a label, this is some very old legacy.
But even with this, it is looks better than on the screenshot from the pull description

@Fedik
Copy link
Member

Fedik commented May 3, 2017

upd: In theory, for a Spacer field we can force class in "JFormFieldSpacer::renderField", something like field-spacer and then in css use .field-spacer>.control-label{width:auto;}
@Quy what do you think?

@Quy
Copy link

Quy commented May 3, 2017

Sounds good to me in theory. but I am no css guru. :)

@DenysNosov
Copy link
Contributor Author

DenysNosov commented May 3, 2017

@Fedik it's nice idea!

@Fedik
Copy link
Member

Fedik commented May 4, 2017

@joomla-ua you will update the PR?

@infograf768
Copy link
Member

infograf768 commented May 5, 2017

@Fedik
Not sure where to add the class in the library.

@Fedik
Copy link
Member

Fedik commented May 5, 2017

@infograf768
in JFormFieldSpacer field class, add method

public function renderField($options = array())
{
   $options['class'] = empty($options['class']) ? 'field-spacer' : $options['class'] .  ' field-spacer';
   
   return parent::renderField($options);
}

other solution can be, to add a layout (renderLayout) specially for a Spacer field instead of default joomla.form.renderfield

@dgrammatiko
Copy link
Contributor

dgrammatiko commented May 5, 2017

I like the layout approach more

@Fedik
Copy link
Member

Fedik commented May 5, 2017

@dgt41 yeah, but how it should looks like? for not break current markup of the forms 😉

@infograf768
Copy link
Member

infograf768 commented May 6, 2017

I can't get it to work here
we have to overwrite width: 160px; in

.form-horizontal .control-label {
    float: left;
    padding-top: 5px;
    width: 160px; 
}

and the code above does not.

@Fedik
Copy link
Member

Fedik commented May 6, 2017

@infograf768 I have sent pull with updates to @joomla-ua
the configuration page render the fields without use of renderField, that why the code from my previous comment not worked 😉

@dgt41 I think here we can use the class, because the layout it is a "new feature" and cannot be accepted until j3.8, I guess. And people will have ugly backend another half of year 😉
We can do the layout for Spacer field anytime later.

@infograf768
Copy link
Member

infograf768 commented May 6, 2017

Eager to test. We have multiple types of spacers and this should take into account all of them.
I would not consider this such a new feature that it could not go into a 3.7.2 (if no B/C impact on frontend).

Correction: just understood you meant the layout would be a new feature.

Fix control-label width for a spacer field
@Quy
Copy link

Quy commented May 6, 2017

It works. I just noticed under System > Global Configuration > Templates, the following does not require the spacer pr because it is coded differently. Should System > Global Configuration > Media be changed to match?

Supported File Formats
Be careful before changing the file types. Read the tool tips before editing. 

@Fedik
Copy link
Member

Fedik commented May 6, 2017

@Quy that another field type note, that actually use a hack to be out of the ".control-label" 😄
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/fields/note.php#L60

@infograf768
Copy link
Member

infograf768 commented May 6, 2017

Will test tomorrow. 😺

<?php $dataShowOn = ''; ?>
<?php
$dataShowOn = '';
$groupClass = $field->type === 'Spacer' ? ' field-spacer' : '';
Copy link
Member

@infograf768 infograf768 May 6, 2017

Choose a reason for hiding this comment

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

Are you sure we need an uppercase for 'Spacer' ? We do not in the xmls.

Copy link
Member

@Fedik Fedik May 6, 2017

Choose a reason for hiding this comment

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

@infograf768
Copy link
Member

infograf768 commented May 7, 2017

I have tested this item successfully on 21929d6

Thanks. Works great.


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

@infograf768
Copy link
Member

infograf768 commented May 7, 2017

@Fedik
We forgot something.
you should do the same for /layouts/joomla/content/options_default.php to cope with Global Configuration.

For example, I have added in /administrator/components/com_config/model/form/application.xml a new spacer field.

                <field name="spacer1"
			type="spacer"
			class="text"
			label="COM_CONFIG_FIELD_DEFAULT_CAPTCHA_DESC"
		/>

(I used an existing lang string).
And I get this
screen shot 2017-05-07 at 08 48 57

Should be easy

@infograf768
Copy link
Member

infograf768 commented May 7, 2017

diff would be:

diff --git a/layouts/joomla/content/options_default.php b/layouts/joomla/content/options_default.php
index 9b42831..e36532c 100644
--- a/layouts/joomla/content/options_default.php
+++ b/layouts/joomla/content/options_default.php
@@ -26,4 +26,5 @@
 		{
 			$datashowon = '';
+			$groupClass = $field->type === 'Spacer' ? ' field-spacer' : '';
 
 			if ($field->showon)
@@ -34,5 +35,5 @@
 			}
 			?>
-			<div class="control-group"<?php echo $datashowon; ?>>
+			<div class="control-group<?php echo $groupClass; ?>"<?php echo $datashowon; ?>>
 				<?php if (!isset($displayData->showlabel) || $displayData->showlabel) : ?>
 					<div class="control-label"><?php echo $field->label; ?></div>

Fedik and others added 2 commits May 7, 2017
@infograf768
Copy link
Member

infograf768 commented May 7, 2017

I have tested this item successfully on d0e5e57


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

@infograf768
Copy link
Member

infograf768 commented May 7, 2017

@ot2sen
Please test again to get this one in.

@AlexRed
Copy link
Contributor

AlexRed commented May 7, 2017

I have tested this item successfully on d0e5e57

Patch ok for me


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

@ot2sen
Copy link
Contributor

ot2sen commented May 7, 2017

I have tested this item successfully on d0e5e57


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

@ghost
Copy link

ghost commented May 7, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label May 7, 2017
@infograf768 infograf768 added this to the Joomla 3.7.1 milestone May 7, 2017
@infograf768
Copy link
Member

infograf768 commented May 7, 2017

@rdeutz
No need to wait 3.7.2 for that one. Can be merged imho for 3.7.1

@rdeutz
Copy link
Contributor

rdeutz commented May 7, 2017

I will keep 3.7.1 as light as possible, so if it is not without any doubt a bugfix it has to wait for 3.7.2

@infograf768
Copy link
Member

infograf768 commented May 7, 2017

OK, no p

@rdeutz rdeutz modified the milestones: Joomla 3.7.2, Joomla 3.7.1 May 9, 2017
@rdeutz rdeutz modified the milestones: Joomla 3.7.2, Joomla 3.7.3 May 18, 2017
@rdeutz rdeutz removed their assignment May 22, 2017
@rdeutz rdeutz merged commit 89230ad into joomla:staging May 22, 2017
4 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC label May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet