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

CommentFileSizeAuthor
#20 filter_guidelines-2098071-20.patch3.22 KBWim Leers
#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
#16 interdiff.txt755 bytesWim Leers
#10 filter_guidelines-2098071-10.patch3.15 KBWim Leers
#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
#2 spacing.textformat.patch1.86 KBBojhan
#1 filter_guidelines-2098071-1.patch713 bytesWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Assigned: Unassigned » Bojhan
Status: Active » Needs work
FileSize
filter_guidelines-2098071-1.patch713 bytes

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

Bojhan CreditAttribution: Bojhan commented
Status: Needs work » Needs review
FileSize
spacing.textformat.patch1.86 KB

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

Bojhan CreditAttribution: Bojhan commented

Noooo! I changed one piece of text though :)

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Needs work » Needs review

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

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
FileSize
text-format-alignment.patch2.01 KB

Ok, some more alignment issues.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
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

BarisW CreditAttribution: BarisW commented
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
FileSize
no-wysiwyg-after.png62.53 KB
wysiwyg-after.png50.9 KB
wysiwyg-before.png67.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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.89 KB
filter_guidelines-2098071-10.patch3.15 KB

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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +sprint

.

swentel’s picture

swentel CreditAttribution: swentel commented

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

Bojhan CreditAttribution: Bojhan commented

Could this get screens?

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
FileSize
interdiff.txt755 bytes
filter_guidelines-2098071-15.patch3.17 KB

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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
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

Bojhan CreditAttribution: Bojhan commented
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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Assigned: Unassigned » Bojhan
FileSize
Screen Shot 2013-10-28 at 18.49.56.png20 KB
interdiff.txt504 bytes
filter_guidelines-2098071-20.patch3.22 KB

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

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

20: filter_guidelines-2098071-20.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
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

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: -sprint
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript

Status: Fixed » Closed (fixed)

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