incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wavegr...@gmail.com
Subject Re: Review Request: Implementing the feature "New wave with the participants of the current wave"
Date Sun, 30 Sep 2012 10:11:19 GMT


> On Sept. 30, 2012, 2:09 a.m., Angus Turner wrote:
> >
> 
> Angus Turner wrote:
>     sorry my bad, it said
>     'some tests would be great if possible'

I'm not sure if it is possible to add any testing for this since it is mostly UI-driven code.
I couldn't find any tests on wave opening and wave creation but maybe I missed it? I welcome
any suggestions.


- rocklund


-----------------------------------------------------------
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