Updated: Comment #0

Problem/Motivation

Now that we have a WYSIWYG editor in core, it makes little sense to have the "filter guidelines" at the bottom.

(Bojhan and I decided to do this as a sort of "phase 1 of #2004284: Integrate "text formats selector" into CKEditor toolbar".)

Proposed resolution

Remove the filter guidelines for every text format for which a text editor is enabled.

Remaining tasks

Fix the CSS.

User interface changes

No more filter guidelines for a text format if that text format has a text editor enabled.

API changes

None.

#2004284: Integrate "text formats selector" into CKEditor toolbar

Files: 
CommentFileSizeAuthor
#20 filter_guidelines-2098071-20.patch3.22 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]
#20 interdiff.txt504 bytesWim Leers
#20 Screen Shot 2013-10-28 at 18.49.56.png20 KBWim Leers
#16 filter_guidelines-2098071-15.patch3.17 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,742 pass(es).
[ View ]
#16 interdiff.txt755 bytesWim Leers
#10 filter_guidelines-2098071-10.patch3.15 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 59,168 pass(es).
[ View ]
#10 interdiff.txt2.89 KBWim Leers
#8 wysiwyg-before.png67.78 KBBarisW
#8 wysiwyg-after.png50.9 KBBarisW
#8 no-wysiwyg-after.png62.53 KBBarisW
#6 text-format-alignment.patch2.01 KBBojhan
PASSED: [[SimpleTest]]: [MySQL] 58,587 pass(es).
[ View ]
#2 spacing.textformat.patch1.86 KBBojhan
PASSED: [[SimpleTest]]: [MySQL] 58,905 pass(es).
[ View ]
#1 filter_guidelines-2098071-1.patch713 bytesWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,715 pass(es).
[ View ]

Comments

Wim Leers’s picture

Assigned:Unassigned» Bojhan
Status:Active» Needs work
StatusFileSize
new713 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,715 pass(es).
[ View ]

Here's the patch that does the PHP side of things. Things look broken now when the filter guidelines are absent; the CSS must be improved to handle that; Bojhan said he'd like to work on that, so assigning to him :)

Bojhan’s picture

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,905 pass(es).
[ View ]

Here you go, I tried to do a interdiff but I failed somewhere and got in a lot of stuff that wasn't part of this patch.

Status:Needs review» Needs work

The last submitted patch, spacing.textformat.patch, failed testing.

Bojhan’s picture

Noooo! I changed one piece of text though :)

Bojhan’s picture

Status:Needs work» Needs review

#2: spacing.textformat.patch queued for re-testing.

Bojhan’s picture

StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,587 pass(es).
[ View ]

Ok, some more alignment issues.

Bojhan’s picture

Title:Disable filter guidelines for a text format if that text format has a text editor enabled» Disable filter guidelines when you have a WYSIWYG
BarisW’s picture

Title:Disable filter guidelines when you have a WYSIWYG» Disable filter guidelines for a text format if that text format has a text editor enabled
StatusFileSize
new62.53 KB
new50.9 KB
new67.78 KB

The patch works fine, but don't we need the filter tips if a user decides to use a non-wysiwyg filter? Like Restricted HTML?

Before the patch (WYSIWYG):
wysiwyg-before.png

After the patch (WYSIWYG):
wysiwyg-after.png

After the patch - wrong? (NO WYSIWYG):
no-wysiwyg-after.png

Wim Leers’s picture

Assigned:Bojhan» Wim Leers
Status:Needs review» Needs work

#8: oh, yes, definitely. Your screenshots show that the proposed resolution

Remove the filter guidelines for every text format for which a text editor is enabled.

is not being met.

I'll fix that.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new2.89 KB
new3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,168 pass(es).
[ View ]

Aforementioned problem fixed.

Also: I noticed that Bojhan's CSS changes were quite invasive and significantly changed the positioning. This updated patch makes far less CSS changes and keeps everything in the exact same place.

Wim Leers’s picture

Issue tags:+sprint

.

swentel’s picture

unsetting things is not really friendly is it, and we don't tend todo that afaik. Better to use #access => FALSE no ?

Bojhan’s picture

Could this get screens?

Bojhan’s picture

I noticed the margins are changed. In the screenshots of #6 show that the "Text format" label aligns with the WYSIWYG buttons and the same spacing is applied to the help icon + label. That seems to be lost now.

The rest looks great :) The thing I am wondering, if there should be a little ease when you flip between having no WYSIWYG and having a WYSIWYG its now all very sudden.

Wim Leers’s picture

#12: good point. Done.

#13: Heh — that was intentional: the margins are not changed in my patch, they were changed in your patch. I don't think it's a great idea to make the text format <select> align with CKEditor's buttons — what if you use a different style on CKEditor? What if you use another text editor than CKEditor? In any case, changing alignment is completely unrelated to the goal of this issue, so if you want to change that, please do it in a different issue. Let's get this in now :)

Wim Leers’s picture

StatusFileSize
new755 bytes
new3.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,742 pass(es).
[ View ]

Ugh forgot to attach the new patch in response to #12.

Status:Needs review» Needs work
Issue tags:-Usability, -sprint, -Spark, -CKEditor in core

The last submitted patch, filter_guidelines-2098071-15.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
Issue tags:+Usability, +sprint, +Spark, +CKEditor in core

#16: filter_guidelines-2098071-15.patch queued for re-testing.

Bojhan’s picture

Assigned:Wim Leers» Unassigned

The margins look bad now, I'd like to fix it. I think the change that I made looks fine with and/or without CKEditor. Unless you feel really passionate about the few px margin change, can we please just fix it here. I'd like to introduce a design change that doesn't require followups.

Wim Leers’s picture

Assigned:Unassigned» Bojhan
StatusFileSize
new20 KB
new504 bytes
new3.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]

Alright then. Please review!
Screen Shot 2013-10-28 at 18.49.56.png

Bojhan’s picture

Status:Needs review» Reviewed & tested by the community

Awesome, thanks! This looks soo much cleaner.

Small followup would be to fix the ? icon to be retina ready.

Wim Leers’s picture

#21: Sure, go ahead :) That's entirely unrelated to this though.

Wim Leers’s picture

Issue summary:View changes

Updated issue summary.

Wim Leers’s picture

webchick’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Fixed

This might be my favourite patch of all time. ;)

Committed and pushed to 8.x. YEAH! Down with confusing, verbose, and unnecessary crap!

Wim Leers’s picture

Issue tags:-sprint
nod_’s picture

Issue tags:+JavaScript

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.