Ordered list support #257
Ordered list support #257
Conversation
…andled. small code factorisation. Added related CSS
|
I will try and test this asap -----Original Message----- Hi ! I implemented ordered list support and added the related icon in the toolbar. It automatically adjusts the numbering while adding and ((de)indenting) entries. You can merge this Pull Request by running: git pull https://github.com/jtlebi/etherpad-lite ordered-list Or you can view, comment on it, or merge it online at: https://github.com/Pita/etherpad-lite/pull/257 -- Commit Summary --
-- File Changes -- M static/css/iframe_editor.css (19) -- Patch Links -- https://github.com/Pita/etherpad-lite/pull/257.patch Reply to this email directly or view it on GitHub: |
|
I talked with jtlebi on the pad and we looked at the issue with the first numbers of lists longer than 9 being cut off. I suggested using list-style-position:inside which does fix the issue, however it also means that the text after the list gets indented a little so they do not line up so the other solution they suggested was increase the left padding. I am not sure which one is better. Also the list type created in the domline.js appendSpan needs changing from ul to ol for ordered list because the start attribute is not used by Chrome or IE for ul lists. The quick and easy solution jtlebi suggested would be in that function you just add a if(listType === "number") but the code for lists could be refactored a bit to make it better. I have been looking through it to see how much work that would be. |
|
EDIT: forget it, it does not work, I did it with a "number" type detection. Not clean at all but it works. As a quick and dirty (tm) fix, I had a silly idea : I switched everything from "ul" to "ol". This must not be a long term fix. Actually, I'd like to refactor the list handling logic as it has evolved quite a bit recently. Could you test this on your browsers please ? I updated the code on my server but I didn't commit yet. |
|
I can see no changes on htmlexport and timeslider. Did you test if the ordered list are working there? |
|
Not yet. I actually focused on the UI interraction |
|
any news on that? |
|
Not on my side. I have a big project currently running so no time for open source for the next days. By the way, the html export code looks very different from the ace2 rendering code. and I have not been able to do a quick fix. Would it be possible to merge this code for the moment so that anybody could help fixing "html export" ? Otherwise, i'll try to get a deeper look at it next week end if I do not have to much work. |
|
I'm not a fan of merging unfinished features. This isn't on the list for 1.1 so it would be an extra. I'm happy to merge it when its ready |
|
If I could emit a suggestion, it would be to deal with duplicated code. Appart from the comments, files blablabla.js and blablabla_client.js are the same. To fix the timeslider, I just had to copy paste the code from one to the other :/ |
|
By the way, this feature is still un-finished. My TODO:
|
|
I have a strange behaviour that I do not know how to fix : adding any char to a numbered list removes the list attribute. It does not happend for unordered lists. And this is very weird since the huge majority of the code is shared. I already double checked the non-shared parts but I have no clue. Any idea ? |
…ly triggered on delete and return key and executed only when necessary
|
All the list user interraction should be OK now. Can anyone test and confirm please ? I really tried to make things as intuitive as possible. |
|
looks good. But the htmlexport code is still untouched. If you manage to update this, this feature will make it to 1.1 |
|
The indent of ordered and unorderd elements is very high. I don't like that |
|
The HtmlExport is the next item on my todo :) When developing the list numbering, I noticed that the "1" of "10" was Le 09/12/2011 17:39, Peter 'Pita' Martischka a écrit :
Jean-Tiare LE BIGOT |
|
Was intuitive for me, tested a bunch of scenarios, worked well. Good work! -----Original Message----- All the list user interraction should be OK now. Can anyone test and confirm please ? I really tried to make things as intuitive as possible. http://www.jtlebi.fr:6001/p/test2 Reply to this email directly or view it on GitHub: |
|
in the first commit, I suggest to indent the whole text pad to keep the relative indentation between text and lists as consistent as possible. There is currently a limitation in the HTMLexport code: it can not handle mixed types on the same level of indentation. see my test case in http://www.jtlebi.fr:6001/p/test1 Sadly, It would require a lot of work to fix. Do you need it before merge ? |
|
Timeslider works, Htmlexport works too - great work Only some small things I would like to change: |
|
Fantastic work :) I'm testing in IE now -----Original Message----- Timeslider works, Htmlexport works too - great work Only some small things I would like to change:
Reply to this email directly or view it on GitHub: |
|
@Pita it is because by default numbers in lists sit outside the content flow and when you reached double digits the numbers on the list were overlapping with the line numbers bar. The alternative solution to that is add list-style-position:inside, but that when you hit double digits the lists look like this: 1. Test
2. Test
3. Test
4. Test
5. Test
6. Test
7. Test
8. Test
9. Test
10. TestSee how when you move from 9 to 10 the text does not line up? The choice is indenting the pad more like the current version or having the text on numbered lists not line up when the number of digits changes. |
|
where can I test this ? |
|
@Wikinaut @johnyma22 see here http://pitapoison.de:9001/ @ja-jo I found a weird bug. When you create a ordered list and press Enter it deletes the ordered list sign. |
|
IE7 and IE8 work pretty well for me in IETester |
|
I would suggest putting this functionality up on one of our dev sites such as TypeWithMe for a week and seeing how people get on and if any bugs are reported prior to merging into master? |
|
One thing I would expect to happen is that if:
*These steps are not required for Microsoft Word to assume that this is part of an ordered list. |
|
@Pita I think the bug you are reporting is actually an intentional feature. In Gdoc, when you press enter in a list item, there i 2 scenarios :
|
|
@johnyma22 Your idea for the numbers sounds very interesting but is that necessary to implement it right in a first version ? Btw, i'm on the IRC to discuss this :) It sounds wise to me to first launch it on a "beta server" and see how this is used before merging. Ther may be forgotten use cases ! |
|
I confirm that ordered lists do work in Firefox and IE8. One question: when marking lines of an ordered (or an unordered) list, and deselecting "ordered list" (or unordered list), I expected that the first indenting is removed, but it is not removed. |
|
Another remark: |
|
Le 11/12/2011 22:12, Wikinaut a écrit :
In my opinion, keeping the relative indentation is more intuitive to the Anyway, I'm open to suggestions on this point. :)
Jean-Tiare LE BIGOT |
|
I do not insist on that but would suggest that in the case a user marked lines of an existing "list", the "list" bullet or number is removed and the first level of indentation is removed. |
|
@Wikinaut I must stress this is really not the thread to comment on the colour picking in IE8. Me and 0ip have both submitted patches for the IE8 bug, feel free to find them and place a pull request with it fixed. |
|
@jtlebi I tested Firefox, Chrome and IE8, and found your enhancement working. Updated on 20.12.2011: However, there's is this core code problem on IE8:
The problem is apparent in http://pitapoison.de:9001/p/test (core plus ordered list) and also in http://beta.etherpad.org (core) As Jtlebi has mentioned below, this appears tp be a core code problem, and not a problem of the ordered-list enhancement, which is subject of this pull request discussion. For this bug I filed https://github.com/Pita/etherpad-lite/issues/292 "Unordered list function broken in IE8: when clicking to add a list in the middle of the pad, the cursor jumps to the top left position of the pad". |
|
Hi all, Sorry for the delay, I had very few spare time these day. starting a new job :) Anyway, I was able to reproduce this bug in IE8 but it is not specific to my modifications as it also occurs on http://beta.etherpad.org for bullet lists. To limit interferences between patches, I suggest to fill another specific bug report. Sadly I'm not a Microsoft product compatibility expert. Could anyone help ? By the way, Thanks @Wikinaut for your heavy/extensive testing ! |
|
For this core code bug I filed https://github.com/Pita/etherpad-lite/issues/292 "Unordered list function broken in IE8: when clicking to add a list in the middle of the pad, the cursor jumps to the top left position of the pad". |
|
@johnyma22 @Pita What is the status of this pull request ? As the time goes it will become hard to merge it as it adds features to core functions :/ Btw, if the merge requires some editions, I have a few hours free this week to help. Merry Christmas and Happy New Year :) |
|
Status is: I want to see the jumping cursor in IE8 issue resolved then we are doing final tests and then merge. Aiming for mid January. that okay with you? |
|
Ooooops ! It's very much like I've produced a Huuuuuuge diff just to fix a conflict :/ Anyway, I merged upstream into my work in order to fix conflicts. The only conflict was in pad.html and caused by commit Pita/etherpad-lite@12fc822 Hopefully, it was pretty easy to fix. |
|
Btw, I did a very quick test with this code and it seams to work pretty well :) |
|
Manually merged, all credit to @jtlebi for making this a possibility :) Sorry about the delay in merging. It's been a busy few weeks! |
This upgrade should be backward compatible, but still suffers form major vulnerabilities in its https-proxy-agent transitive dependency (see https://www.npmjs.com/advisories/1184). Changelog: - https://github.com/npm/cli/releases 6.12.0 (2019-10-08): Now npm ci runs prepare scripts for git dependencies, and respects the --no-optional argument. Warnings for engine mismatches are printed again. Various other fixes and cleanups. BUG FIXES 890b245dc #252 ci: add dirPacker to options (@claudiahdz) f3299acd0 #257 npm.community#4792 warn message on engine mismatch (@ruyadorno) bbc92fb8f #259 npm.community#10288 Fix figgyPudding error in npm token (@benblank) 70f54dcb5 #241 doctor: Make OK more consistent (@gemal) FEATURES ed993a29c #249 Add CI environment variables to user-agent (@isaacs) f6b0459a4 #248 Add option to save package-lock without formatting Adds a new config --format-package-lock, which defaults to true. (@bl00mber) DEPENDENCIES 0ca063c5d npm-lifecycle@3.1.4: fix: filter functions and undefined out of makeEnv (@isaacs) 5df6b0ea2 libcipm@4.0.4: fix: pack git directories properly (@claudiahdz) respect no-optional argument (@cruzdanilo) 7e04f728c tar@4.4.12 5c380e5a3 stringify-package@1.0.1 (@isaacs) 62f2ca692 node-gyp@5.0.5 (@isaacs) 0ff0ea47a npm-install-checks@3.0.2 (@isaacs) f46edae94 hosted-git-info@2.8.5 (@isaacs) TESTING 44a2b036b #262 fix root-ownership race conditions in meta-test (@isaacs) 6.11.3 (2019-09-03): Fix npm ci regressions and npm outdated depth. BUG FIXES 235ed1d28 #239 Don't override user specified depth in outdated. Restores ability to update packages using --depth as suggested by npm audit. (@G-Rath) 1fafb5151 #242 npm.community#9586 Revert "install: do not descend into directory deps' child modules" (@isaacs) cebf542e6 #243 npm.community#9720 ci: pass appropriate configs for file/dir modes (@isaacs) DEPENDENCIES e5fbb7ed1 read-cmd-shim@1.0.4 (@claudiahdz) 23ce65616 npm-pick-manifest@3.0.2 (@claudiahdz) 6.11.2 (2019-08-22): Fix a recent Windows regression, and two long-standing Windows bugs. Also, get CI running on Windows, so these things are less likely in the future. DEPENDENCIES 9778a1b87 cmd-shim@3.0.3: Fix regression where shims fail to preserve exit code (@isaacs) bf93e91d8 npm-package-arg@6.1.1: Properly handle git+file: urls on Windows when a drive letter is included. (@isaacs) BUGFIXES 6cc4cc66f escape args properly on Windows Bash Despite being bash, Node.js running on windows git mingw bash still executes child processes using cmd.exe. As a result, arguments in this environment need to be escaped in the style of cmd.exe, not bash. (@isaacs) TESTS 291aba7b8 make tests pass on Windows (@isaacs) fea3a023a travis: run tests on Windows as well (@isaacs) 6.11.1 (2019-08-20): Fix a regression for windows command shim syntax. 37db29647 cmd-shim@3.0.2 (@isaacs) v6.11.0 (2019-08-20): A few meaty bugfixes, and introducing peerDependenciesMeta. FEATURES a12341088 #224 Implements peerDependenciesMeta (@arcanis) 2f3b79bba #234 add new forbidden 403 error code (@claudiahdz) BUGFIXES 24acc9fc8 and 45772af0d #217 npm.community#8863 npm.community#9327 do not descend into directory deps' child modules, fix shrinkwrap files that inappropriately list child nodes of symlink packages (@isaacs and @salomvary) 50cfe113d #229 fixed typo in semver doc (@gall0ws) e8fb2a1bd #231 Fix spelling mistakes in CHANGELOG-3.md (@XhmikosR) 769d2e057 npm/uid-number#7 Better error on invalid --user/--group configs. This addresses the issue when people fail to install binary packages on Docker and other environments where there is no 'nobody' user. (@isaacs) 8b43c9624 nodejs/node#28987 npm.community#6032 npm.community#6658 npm.community#6069 npm.community#9323 Fix the regression where random config values in a .npmrc file are not passed to lifecycle scripts, breaking build processes which rely on them. (@isaacs) 8b85eaa47 save files with inferred ownership rather than relying on SUDO_UID and SUDO_GID. (@isaacs) b7f6e5f02 Infer ownership of shrinkwrap files (@isaacs) 54b095d77 #235 Add spec to dist-tag remove function (@theberbie) DEPENDENCIES dc8f9e52f pacote@9.5.7: Infer the ownership of all unpacked files in node_modules, so that we never have user-owned files in root-owned folders, or root-owned files in user-owned folders. (@isaacs) bb33940c3 cmd-shim@3.0.0: 9c93ac3 #2 npm#3380 Handle environment variables properly (@basbossink) 2d277f8 #25 #36 #35 Fix 'no shebang' case by always providing $basedir in shell script (@igorklopov) adaf20b #26 Fix $* causing an error when arguments contain parentheses (@satazor) 49f0c13 #30 Fix paths for MSYS/MINGW bash (@dscho) 51a8af3 #34 Add proper support for PowerShell (@ExE-Boss) 4c37e04 #10 Work around quoted batch file names (@isaacs) a4e279544 npm-lifecycle@3.1.3 (@isaacs): fail properly if uid-number raises an error 7086a1809 libcipm@4.0.3 (@isaacs) 8845141f9 read-package-json@2.1.0 (@isaacs) 51c028215 bin-links@1.1.3 (@isaacs) 534a5548c read-cmd-shim@1.0.3 (@isaacs) 3038f2fd5 gentle-fs@2.2.1 (@isaacs) a609a1648 graceful-fs@4.2.2 (@isaacs) f0346f754 cacache@12.0.3 (@isaacs) ca9c615c8 npm-pick-manifest@3.0.0 (@isaacs) b417affbf pacote@9.5.8 (@isaacs) TESTS b6df0913c #228 Proper handing of /usr/bin/node lifecycle-path test (@olivr70) aaf98e88c npm-registry-mock@1.3.0 (@isaacs)
Hi !
I implemented ordered list support and added the related icon in the toolbar. It automatically adjusts the numbering while adding and ((de)indenting) entries.
There still are an issue since deleting an entry is not immediately taken into account. I guess I should add a hook into "doDeleteKey()" but I'm not yet sure of how do to it properly.
Btw, you are welcome to test it here: http://www.jtlebi.fr:6001/p/test1