incubator-wave-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Coleman" <padster...@gmail.com>
Subject Re: Review Request: Implementing the feature "New wave with the participants of the current wave"
Date Mon, 22 Oct 2012 14:17:37 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7353/#review12654
-----------------------------------------------------------


Really nice feature :) One thing to also to consider in the future is something I added in
GWave but didn't migrate to the new client but worked ok,
which was drag-drop support for this feature; I'm not sure apache wave's DnD support, but
it's nice to have the ability to drag things with profiles attached
(either a profile picture, a participants widget, or even a search panel wave) onto the new
wave button, to create a wave with those people.


src/org/waveprotocol/box/webclient/client/StageTwoProvider.java
<https://reviews.apache.org/r/7353/#comment26992>

    small nit: looks like this could be final?



src/org/waveprotocol/box/webclient/client/WebClient.java
<https://reviews.apache.org/r/7353/#comment26991>

    small nit: I'd expand the documentation:
    "the participants to add to the newly created wave, in addition to the creator."
    It was hard to tell whether the creator needed to be in the set, or of they were added
independantly.
    
    Possibly the variable name 'participants' could be 'otherParticipants' or similar.



src/org/waveprotocol/wave/client/wavepanel/view/dom/full/ParticipantsViewBuilder.java
<https://reviews.apache.org/r/7353/#comment26989>

    Long HTML insertions like this are often troublesome in terms of XSS attacks and general
readability.
    My recommendation would be to have something like openButton() and closeButton() which
take the css, type, title etc, like the other open/close methods.


- Patrick Coleman


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


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