----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7353/#review12057 -----------------------------------------------------------
Hi Thanks for doing this! I think we can leave the button on participants panel. If you don't have any appropriate animation, we can just make it as text button, like "new wave". Optimally, clicking this button will open a popup that will present all current wave participants with check boxes selected, and then you will have an option to unselect some and click on "create" which will create a new wave with selected participants. src/org/waveprotocol/box/webclient/client/StageTwoProvider.java <https://reviews.apache.org/r/7353/#comment25711> Please terminate with full stop and put "null" inside code block. src/org/waveprotocol/box/webclient/client/WebClient.java <https://reviews.apache.org/r/7353/#comment25703> Please terminate the comment with full stop. I think the "null" can be put inside {@code } block, i.e. null -> {@code null} src/org/waveprotocol/box/webclient/client/events/WaveCreationEvent.java <https://reviews.apache.org/r/7353/#comment25705> I think we can implement passing the list of participants similar to passing the waveId in WaveSelectionEvent. I mean we can create two constructors in WaveCreationEvent - one without arguments, and one that takes the participants. Then we can create new instances of the WaveCreationEvent using constructors - similar to WaveSelectionEvent instead of reusing the same static instance as in current implementation. This would also address the issue with the name of static factory method which is createNewEventWithParticipants although it doesn't create any new instances. src/org/waveprotocol/wave/client/wavepanel/view/View.java <https://reviews.apache.org/r/7353/#comment25708> Can we change the type name to NEW_WAVE_WITH_PARTICIPANTS ? src/org/waveprotocol/wave/client/wavepanel/view/dom/DomAsViewProvider.java <https://reviews.apache.org/r/7353/#comment25707> Can we change the method name from fromNewParticipantWaveButton to fromNewWaveWithParticipantsButton ? src/org/waveprotocol/wave/client/wavepanel/view/dom/full/Participants.css <https://reviews.apache.org/r/7353/#comment25709> same, .newParticipantWavePanel -> .newWaveWithParticipantsPanel src/org/waveprotocol/wave/client/wavepanel/view/dom/full/Participants.css <https://reviews.apache.org/r/7353/#comment25710> ditto - Yuri Zelikov On Sept. 29, 2012, 11:53 a.m., rocklund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7353/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2012, 11:53 a.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/wavepanel/impl/edit/ParticipantController.java > a6eeb53 > 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/client/wavepanel/view/dom/full/new_prts_wbutton.png > PRE-CREATION > > 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 > > > Screenshots > ----------- > > > https://reviews.apache.org/r/7353/s/1/ > > > Thanks, > > rocklund > >
