----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7353/#review12634 -----------------------------------------------------------
Great patch - just minor comments. Thanks for doing this! The only issue with "New Wave" button on Firefox - it wraps the words on two lines which makes the button too big. src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantSelectorWidget.java <https://reviews.apache.org/r/7353/#comment26971> Please remove the google header src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantSelectorWidget.java <https://reviews.apache.org/r/7353/#comment26972> Why 20 px? Can we extract this to a constant? src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantSelectorWidget.ui.xml <https://reviews.apache.org/r/7353/#comment26973> Please remove Google Copyright header. src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantWidget.java <https://reviews.apache.org/r/7353/#comment26974> Please remove Google copyright header. src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantWidget.ui.xml <https://reviews.apache.org/r/7353/#comment26975> Remove copyright - Yuri Zelikov On Oct. 19, 2012, 4:54 p.m., rocklund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7353/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2012, 4:54 p.m.) > > > Review request for wave. > > > Description > ------- > > I've started to create the functionality me and my friends used a lot in > Google Wave - to be able to create a new wave with the participants from an > open wave. (I could not find a JIRA for this) > > The functionality of the patch is working as intended. There is currently a > few things that I would like some input on: > > UI (See the screenshot) > 1. The icon I use is currently only a placeholder icon. Is anyone able to > create a better icon or any ideas of how it can look and I can try to arrange > one? Or is it better to just display plain text? > 2. Is it OK to add this functionality as an icon/text beside the "Add > participant" button for now or should it be placed in a submenu instead that > can eventually contain more functionality? Maybe at least the button should > be placed to the far right edge of the participant panel? > > Implementation > 3. I currently just added the participant addition code into the install > method of the StageTwoProvider. It feels like it might not be the best spot. > Any feedback on where it could be placed? > 4. In the WaveCreationEvent class I only create one > "CREATE_NEW_WAVE_WITH_PARTICIPANTS"-event where I update the participant > pointer. Maybe it is safer to create a new event for every call? > 5. Any other implementation feedback? > > Thanks. > > > Diffs > ----- > > src/org/waveprotocol/box/webclient/client/StageTwoProvider.java 7a6c8ec > src/org/waveprotocol/box/webclient/client/StagesProvider.java 9d83269 > src/org/waveprotocol/box/webclient/client/WebClient.java 863ae6c > src/org/waveprotocol/box/webclient/client/events/WaveCreationEvent.java > 95c317b > > src/org/waveprotocol/box/webclient/client/events/WaveCreationEventHandler.java > adc57f9 > src/org/waveprotocol/wave/client/StageThree.java 5a8de4d > > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantController.java > a6eeb53 > > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantSelectorWidget.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantSelectorWidget.ui.xml > PRE-CREATION > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantWidget.java > PRE-CREATION > > src/org/waveprotocol/wave/client/wavepanel/impl/edit/ParticipantWidget.ui.xml > PRE-CREATION > src/org/waveprotocol/wave/client/wavepanel/view/View.java c770b36 > src/org/waveprotocol/wave/client/wavepanel/view/dom/DomAsViewProvider.java > 64e7f79 > src/org/waveprotocol/wave/client/wavepanel/view/dom/FullStructure.java > e848c5f > src/org/waveprotocol/wave/client/wavepanel/view/dom/full/Participants.css > 66f1836 > > src/org/waveprotocol/wave/client/wavepanel/view/dom/full/ParticipantsViewBuilder.java > 735ec04 > src/org/waveprotocol/wave/client/wavepanel/view/dom/full/TypeCodes.java > ba8bc7a > src/org/waveprotocol/wave/concurrencycontrol/wave/CcBasedWavelet.java > e87b1e0 > src/org/waveprotocol/wave/model/conversation/Conversation.java cff46ed > src/org/waveprotocol/wave/model/conversation/WaveletBasedConversation.java > 62c5364 > src/org/waveprotocol/wave/model/wave/Wavelet.java a463d45 > src/org/waveprotocol/wave/model/wave/opbased/OpBasedWavelet.java e99067c > test/org/waveprotocol/wave/model/wave/opbased/OpBasedWaveletTestBase.java > 1ffd8f3 > > Diff: https://reviews.apache.org/r/7353/diff/ > > > Testing > ------- > > Tested manually on a local server both the implementation and the button > placement/adjustment when adding/removing participant and shrinking the > window size > Added one test case for adding a set of participants to a wave > > > Screenshots > ----------- > > > https://reviews.apache.org/r/7353/s/7/ > > > Thanks, > > rocklund > >
