List conversion #33
List conversion #33
Conversation
|
VIDEO: https://drive.google.com/a/policystat.com/file/d/0B8McUPMi9l7mZjJ2VTJIRUlvcEk/view?usp=sharing About the above video: Ignore the fact that my cursor is way off to the right of where it actually is, and the fact that this is all currently being run off of an additional 'blank' button. The first process converts all top-level header items to autonumbered headings, and then outdents the structure so they are all outside of a list. This has the consequence of turning all second-level items into their own separate new lists. Running the process again on these lists then creates new top level headers and the structure was lost, I've accounted for this by checking if there was a previous header before running the process and if so running at the next higher level. This appears to function fine so far, but will require lots more testing to find edge cases. |
|
@BDeveau What needs review exactly? |
|
Currently just making sure I'm going in the right direction, that the before and after results shown in the video are what we wanted to have happen. |
|
Ok, I see. Looks ok, but I don't have any research on if this is something users need. Can we bring it to the Hipchat room to find out where the research/requests are for this functionality? |
| @@ -307,7 +308,8 @@ | |||
| editor.elementPath().block.addClass(editor.config.autonumberBaseClass); | |||
| setStyle(editor, editor.elementPath().block, editor.config.autonumberCurrentStyle); | |||
| } | |||
|
|
|||
| } else if (editor.elementPath().block.is({li: 1})) { | |||
There was a problem hiding this comment.
It's a bit confusing how this works, but it might be better to use blockLimit to determine if we're in a list. In this case I think it might only work if there's no nested block (table or paragraph inside a list item)
But that might be intended. I'm not sure.
structuredheadings/plugin.js
Outdated
| exec: function (editor) { | ||
|
|
||
| var selectArea = function (startElement, endElement, encompass) { | ||
| encompass = encompass ? encompass : false; |
There was a problem hiding this comment.
if this is bool, can't it just be encompass || false ?
There was a problem hiding this comment.
true.. that is cleaner
| range.setEndAfter(endElement, 0); | ||
| } else { | ||
| range.setStart(startElement, 0); | ||
| range.setEnd(endElement, 0); |
There was a problem hiding this comment.
I'm not sure, but given the 0 index on the startElement's offset , i would have expected setEnd to be the last-1 offset of endElement. Maybe this needs more explanation
There was a problem hiding this comment.
using setStart and setEnd at the same point is placing the cursor, rather than a full selection. using setStart and setEnd on the same element, same offset is the equivalent of '<h1>^Heading</h1>, just enough to place cursor location to allow me to operate on that block element. Setting encompass to true would then use the setStartBefore and setEndAfter which would create the actual selection.
structuredheadings/plugin.js
Outdated
| /* The Worker */ | ||
| var processList = function (listElement, level, deep) { | ||
| level = level ? level : 0; | ||
| deep = deep ? deep : false; |
There was a problem hiding this comment.
same comments about bools here
structuredheadings/plugin.js
Outdated
| processList(node, level + 1); | ||
| return false; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
you could also just pull return false out as the last statement in this function (after the conditional)
| // editor.getSelection().getRanges()[0].getTouchedEndNode(), | ||
| // editor.getSelection().getRanges()[0].getTouchedEndNode() | ||
| // ); | ||
| // editor.getSelection().scrollIntoView(); |
There was a problem hiding this comment.
intentionally commented?
There was a problem hiding this comment.
Depending on if Wes prefers to have the new selection at the first or last element of the converted list. commented and sitting, pending UX review.
There was a problem hiding this comment.
Hmm I've apparently missed much of this review in my scramble to get other things done before my vacation. Does this card belong in the UX/UI review column then?
|
|
||
| //get the current list root element from the selection | ||
| var listRoot = editor.elementPath().contains(["ol", "ul"], false, true); | ||
| var deep = false; //don't deep process |
There was a problem hiding this comment.
maybe I need more time with this, but it doesn't seem immediately obvious what "deep process" means here
There was a problem hiding this comment.
I have it in there in case Wes changes his mind how he wants the conversion to work. setting deep true will enable recursion, and convert the entire list from head to toe instead of just the "first level"
| "<ul><li>SubListItem</li></ul>", updatedContent, | ||
| "Sublist in tact" | ||
| ); | ||
|
|
There was a problem hiding this comment.
we could probably use some tests with things like:
- a formatting element inside a list item
- more complex list item content
- the really terrible list item that has text before and after an inner nested list (for ex
<li>foo<ul>...</ul>bar</li>which ckeditor does poorly with sometimes
There was a problem hiding this comment.
Another thought: content whihc is allowed within list items, but not within headings
The ability to automatically convert lists into structured headings