> 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. > > Yuri Zelikov wrote: > 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 Zelikov wrote: > I noticed that it happens only when compiled in the dev mode using > compile-gwt-dev, it seems to work fine if compiled with compile-gwt > > Vicente J. Ruiz Jurado wrote: > Works to me also with compile-gwt-dev (In Chrome). > > Yuri Zelikov wrote: > Tried again and I can reproduce it when compiled with compile-gwt-dev. > Just make sure to use both new controls with overlapping boundaries. > > Vicente J. Ruiz Jurado wrote: > Sorry, I cannot reproduce it. I'm using chrome because WebClientDev is > only enabled for "safari". > > Ali Lown wrote: > Via extremely lazy consensus (and since this is a useful addition), I > have now committed it. > > Yuri Zelikov wrote: > Ali, did you try to reproduce the issue I described? > > Ali Lown wrote: > Yep, and I was unable to. Setting colours seemed to work fine in > compile-gwt-dev mode here. (Chrome 27.0.1430.0) > > Yuri Zelikov wrote: > Great, thanks!
Thanks Ali. (Minor comment: For the next time, as I'm already an Apache wave commiter, I prefer to commit myself my contributions) - Vicente J. ----------------------------------------------------------- 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 > >
