incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zachary “Gamer_Z.” Yaro <zmy...@gmail.com>
Subject Re: Review Request: Implementing the feature "New wave with the participants of the current wave"
Date Sun, 30 Sep 2012 04:38:13 GMT
While adding that feature would be fantastic, may I ask why you chose to
add a button for it instead of putting it in a menu, as Gwave did?

—Zachary “Gamer_Z.” Yaro


On Sat, Sep 29, 2012 at 10:19 PM, Angus Turner <hi@theangus.org> wrote:

>
>
> > On Sept. 30, 2012, 2:09 a.m., Angus Turner wrote:
> > >
>
> sorry my bad, it said
> 'some tests would be great if possible'
>
>
> - Angus
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7353/#review12045
> -----------------------------------------------------------
>
>
> 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
> >
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message