> On March 21, 2013, 2:05 p.m., Ali Lown wrote: > > Tested locally and seems to work as expected. > > > > Code is fine, allows extending to more complex colour pickers in future if > > needed. (Though not sure if this is really necessary). > > Vicente J. Ruiz Jurado wrote: > Another simple color picker that I want to develop is one that remembers > the previous color selected (a GDocs do). In kune.cc we have another gwt > color picker, but we are not very happy to add it to Wave (you can test it > there). > > In the future, with other versions of GWT, we can use some other color > pickers: > > http://www.subshell.com/en/subshell/blog/Implementing-a-Color-Picker-Dialog-With-Canvas-and-GWT100.html > > Any other comment to this review? > > Ali Lown wrote: > Not from me.
I got a shiny when tried to test this. Stack trace: Token: 1365103711828 Repairer triggered, handleMissing Unknown.$collect (JsArrayString.java:42) Unknown.fillInStackTrace_2 (StackTraceCreator.java:147) Unknown.fillInStackTrace_0 (StackTraceCreator.java:387) Unknown.fillInStackTrace (Throwable.java:72) Unknown.$$init_10 (Throwable.java:46) Unknown.Throwable_2 (Throwable.java:56) Unknown.Error_2 (Error.java:30) Unknown.AssertionError_1 (AssertionError.java:51) Unknown.throwAssertionError_Object (Exceptions.java:66) Unknown.$handleMissing (Repairer.java:128) Unknown.$handle (Repairer.java:106) Unknown.$findNodeletWithOffset_0 (ContentTextNode.java:506) Unknown.$implSplitText (ContentTextNode.java:235) Unknown.$splitText_3 (ContentTextNode.java:213) Unknown.$splitText_2 (ContentRawDocument.java:221) Unknown.splitText_2 (ContentRawDocument.java:218) Unknown.splitText_1 (PersistentContent.java:388) Unknown.$splitCurrent (IndexedDocumentImpl.java:1256) Unknown.$splitText_0 (IndexedDocumentImpl.java:1477) Unknown.$splitText_1 (ContentDocument.java:1066) Unknown.splitText_0 (ContentDocument.java:1063) Unknown.ensureNodeBoundaryReturnNextNode (DocHelper.java:721) Unknown.$ensureNodeBoundary (AnnotationPainter.java:370) Unknown.$doRun (AnnotationPainter.java:193) Unknown.execute_37 (AnnotationPainter.java:89) Unknown.$workUnit (BrowserBackedScheduler.java:276) Unknown.$workAll (BrowserBackedScheduler.java:323) Unknown.$workSlice (BrowserBackedScheduler.java:371) Unknown.run_17 (BrowserBackedScheduler.java:44) Unknown.run_18 (GwtSimpleTimer.java:49) Unknown.fire (Timer.java:141) Unknown.anonymous (Timer.java:60) Unknown.apply (Impl.java:168) Unknown.entry0 (Impl.java:214) Unknown.anonymous (Impl.java:57) - Yuri ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9841/#review18206 ----------------------------------------------------------- On March 11, 2013, 12:13 p.m., Vicente J. Ruiz Jurado wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9841/ > ----------------------------------------------------------- > > (Updated March 11, 2013, 12:13 p.m.) > > > Review request for wave, Michael MacFadden, Yuri Zelikov, Ali Lown, and > hegsie. > > > Description > ------- > > This patch add the possibility to set color and background colors of wave > text. It's something that our users in kune were asking for and finally I > decided to implement it. > > Now that I'm filling this review, I see that there is some related bug and > some work in progress in #WAVE-270 (Jan-2012). I hope that can be reused and > integrated with this patch (my approach allow to integrate other color > pickers). Sorry hegsie! Feel free to remix/review/improve this patch. > > You should put the new icons in > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/images/edit/ > > > This addresses bug WAVE-270. > https://issues.apache.org/jira/browse/WAVE-270 > > > Diffs > ----- > > src/org/waveprotocol/wave/client/testing/UndercurrentHarness.java 25abe0c > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/EditToolbar.css > 8bf9970 > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/EditToolbar.java > 3e196b5 > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/EditorToolbarResources.java > b5e616e > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/AbstractColorPicker.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/ColorHelper.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/ColorPopup.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/ComplexColorPicker.css > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/ComplexColorPicker.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/OnColorChooseListener.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/SampleCustomColorPicker.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/SimpleColorPicker.css > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/SimpleColorPicker.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/i18n/ColorPickerMessages.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/i18n/ColorPickerMessages_en.properties > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/color/i18n/ColorPickerMessages_es.properties > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/images/edit/backcolor.png > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/toolbar/images/edit/color.png > PRE-CREATION > > Diff: https://reviews.apache.org/r/9841/diff/ > > > Testing > ------- > > Some text editions... setting/unsetting colors. > > > Thanks, > > Vicente J. Ruiz Jurado > >
