----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7353/ -----------------------------------------------------------
(Updated Oct. 22, 2012, 8:41 p.m.) Review request for wave. Changes ------- Thanks for the reviews! I have now updated according to Yuris and Patricks comments. Some comments of my update: * I updated the size of the button so that the words does not wrap in firefox on my machine anymore. * I also removed the 20px negative offset on the popup height calculation since I noticed that I did not need it. * I removed the Google copyright. Should I instead add the Apache copyright header? * About the drag and drop support - it is very nice idea and it would indeed be nice to have. But I don't feel like implementing all that for this patch though :) Thanks. 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 (updated) ----- 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/uibuilder/OutputHelper.java b33d0ff 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 (updated) ------- Tested manually on a local server both the implementation and the button placement/adjustment when adding/removing participant and shrinking the window size. Now tested this in both Google Chrome and Firefox. Added one test case for adding a set of participants to a wave Screenshots ----------- https://reviews.apache.org/r/7353/s/7/ Thanks, rocklund
