Opened 14 years ago
Closed 14 years ago
#7991 closed Bug (fixed)
Labels don't fit the Table Properties dialog in some languages.
Reported by: | Jakub Ś | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.6.1 |
Component: | UI : Dialogs | Version: | 3.6.1 |
Keywords: | Cc: |
Description
In [6979] unit select list has been removed. Now ,for some languages, the labels describing cell padding and cell spacing don’t fit the dialog.
These languages are: Vietnamese, Russian, Lithuanian, Georgian, French, Faroese, Dutch, Czech, Catalan, Bulgarian, Mongolian, Polish
In Firefox and Opera labels go out of dialog. In IE9 part of label is hidden (like overflow:hidden) In Webkit and IE8 scroll bars appear.
In IE6 and IE7 dialog is enlarged to fit the label. In other words it looks nice here.
Attachments (9)
Change History (31)
comment:1 Changed 14 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 14 years ago by
Milestone: | → CKEditor 3.6.1 |
---|
Changed 14 years ago by
Attachment: | 7991.patch added |
---|
comment:3 Changed 14 years ago by
Owner: | set to Garry Yao |
---|---|
Status: | confirmed → review |
comment:4 Changed 14 years ago by
Status: | review → review_passed |
---|
Looks good to me, if anyone sees a problem please reassign the status of the ticket.
comment:5 Changed 14 years ago by
Status: | review_passed → review_failed |
---|
In my opinion the proposed solution is not right if we think of user experience.
We should keep input fields short where we don't expect large numbers.
Changed 14 years ago by
Attachment: | 7991_2.patch added |
---|
comment:6 Changed 14 years ago by
Status: | review_failed → review |
---|
comment:7 Changed 14 years ago by
Right now we had a limitation in the dialog layout that the specified field width is bound with label size, which means short field won't allow a long label fill to stretch the dialog. E.g. having the following definition:
type : 'text', id : 'txtCellSpace', style : 'width:3em'
Constraint both the label and input to "3em" width.
Propose a new "inputStyle" definition entry which makes it possible to have fine-grid control over style on both input element and the label.
comment:8 follow-up: 9 Changed 14 years ago by
There's already a "controlStyle" option: http://dev.ckeditor.com/ticket/5927
comment:9 Changed 14 years ago by
Status: | review → review_passed |
---|
Replying to alfonsoml:
There's already a "controlStyle" option: http://dev.ckeditor.com/ticket/5927
That's different, as controlStyle is applied to the input itself while inputStyle is applied to the surrounding <div>. Though we really are missing the docs for controlStyle.
comment:10 follow-up: 12 Changed 14 years ago by
It's gonna be fun to have "inputStyle" applied to a div and "controlStyle" applied to an input.
Are we sure that we want to use those names?
Once it's released we'll be again on the old game "yeah, that's bad, but changing it would break external code. We might change it in the next big rewrite 2 years from now"
comment:11 Changed 14 years ago by
Indeed it might be quite confusing... The patch is good, but using better names for options right from the beginning also sounds reasonable. Alfonso, do you have any suggestions?
comment:12 Changed 14 years ago by
Replying to alfonsoml:
It's gonna be fun to have "inputStyle" applied to a div and "controlStyle" applied to an input.
As you proposed "controlStyle" back in #5927 to introduce an easy way of styling the control element separately from label, exactly of the same purpose with me here (but I didn't know that option and it turns out to have only one usage in the codebase).
I propose to deprecate the "controlStyle" in favor of "inputStyle" for the following reasons:
- The style should goes on the input wrapper (e.g. cke_dialog_ui_input_text) instead of the input element itself, as the wrapper carries part of the input style.
- IMO "inputStyle" get also better adopted to the current code as we already have something like "getInputElement".
Changed 14 years ago by
Attachment: | 7991_3.patch added |
---|
comment:13 Changed 14 years ago by
@Saar, can you review the changes to the docProps dialog if it breaks anything?
comment:14 Changed 14 years ago by
Using the latest patch, page margins in the "Design" tab in the docProps dialog are no longer centered (to reproduce just type something in the "Top" field).
Changed 14 years ago by
Attachment: | 7991_4.patch added |
---|
comment:15 Changed 14 years ago by
Status: | review_passed → review |
---|
I didn't realize the input field content should also be center aligned...but that would be quite a rare case to be considered.
Changed 14 years ago by
illustration of the dialog field width definition
comment:16 Changed 14 years ago by
comment:17 Changed 14 years ago by
DocProps dialog: actually I do not see any reason why
this.getElement().getParent().setStyle( 'text-align', 'center' );
was used there. Am I wrong or controlStyle : 'text-align: center'
was enough to keep the text aligned to the center?
Regarding controlStyle / labelStyle / inputStyle
(I'll use current names in this post just to avoid confusion). In my opinion they are all useful. Thanks to the controlStyle I believe we can get rid of
function textCenterAlign() { this.getInputElement().setStyle( 'text-align', 'center' ); }
used in the last patch. Being able to style the element perfectly using just the definition I believe is something that Alfonso tried to achieve with #5927.
Alfonso suggested using better names, although I agree that we should not use confusing names, I have no idea what names would be better :|.
One more thing regarding the functionality, I've just made a quick test and noticed some inconsistency (using the 2# patch if I'm correct).
{ type: 'text', id : 'foo1', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', default : 'initial value', labelStyle : 'color:red' }, { type: 'textarea', id : 'foo2', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', default : 'initial value', labelStyle : 'color:red' }, { type: 'select', id : 'foo3', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', labelStyle : 'color:red', items : [ ['foo'] ] }, { type: 'checkbox', id : 'foo4', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', labelStyle : 'color:red' }, { type: 'radio', id : 'foo5', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', labelStyle : 'color:red', items : [ ['foo'] ] }, { type: 'html', id : 'foo6', style : 'width:200px', html : 'foo html', inputStyle : 'border:5px solid red', controlStyle : 'color:blue' }, { id : 'foo7', type: 'file', style : 'width:200px', label : 'label', inputStyle : 'border:5px solid red', controlStyle : 'color:blue', labelStyle : 'color:red' }
Changed 14 years ago by
Attachment: | 7991_styling.png added |
---|
Changed 14 years ago by
Attachment: | 7991_5.patch added |
---|
comment:18 Changed 14 years ago by
Following Wiktor's review, switch the names between 'inputStyle' and 'controlStyle' for better semantics, fixing the above mentioned bugs.
Changed 14 years ago by
Attachment: | test_dialog_fields.js added |
---|
comment:19 Changed 14 years ago by
Another option (just in case you want another idea, but I think that the latest patch is quite better to keep everyone's sanity than the original proposal) would be to call the style for the wrapper div inputWrapperStyle instead of controlStyle.
When the dialog system is reviewed, it should be checked if such div wrapper is really needed or it can be removed, less elements, less things to think about.
Changed 14 years ago by
Attachment: | 7991_apidocs.patch added |
---|
comment:20 Changed 14 years ago by
I've noticed that in fact we're missing the documentation for CKEDITOR.dialog.definition.labeledElement. The attached patch contains changes from Garry's patch to dialogDefinition.js
along with some further improvements.
comment:21 Changed 14 years ago by
Status: | review → review_passed |
---|
The code pieces in the DocProps plugin are there to align the fields in Quirks mode, thus it must remain untoucned.
It's otherwise R+, so I'd opt to commit the patch without these changes (let's also add a comment there to remind what's the purpose of this code piece).
comment:22 Changed 14 years ago by
Component: | General → UI : Dialogs |
---|---|
Resolution: | → fixed |
Status: | review_passed → closed |
Fixed with [7029].
Can't we just resigns all dimension restrictions over the table dialog, which makes l11n just easier and also looks neater.