Make a bunch of improvements to the tips in the review dialog.

 






 

 


  Review Request #13192
  —
  Created Aug. 8, 2023 and submitted 

 




















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  release-6.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This change makes a bunch of improvements to the new tips feature in the

review dialog:


    

  • Adds a hide button to allow people to hide the tips. While it's
    
  probably good for new users, experienced users likely won't want it
    
  cluttering up their screen. The preference for this is stored in a
    
  cookie
    • 

  • Moves the tips box to the top of the review dialog.
    • 

  • Changes it so the tips box is always visible, even when there's review
    
  content.
    • 

  • Adds new content to the tips list (thanks Michelle!).
    • 


 








  


 
 

  
    

  • Toggled the tips view back and forth.
    • 

  • Checked that the tips view shows up in the right place.
    • 

  • Verified that the initial tips view visibility respects the stored
    
  preference.
    • 

  • Ran unit tests.
    • 

  • Ran js-tests.
    • 


 







 









 





  

 


 
 

  



 

  

    

    

  
  
   

     Summary

   

  
  

   


    

     
      
     

    

    
Make a bunch of improvements to the tips in the review dialog.


   


  
 




 













 

  
  


   

    

     

     

     

    

   


   

    

     

     

     

    

   


   

  

 







 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
We should probably change all of the ’ apostrophes to the ' ones that we normally use. And any “ …

      		
      
       maubinmaubin
      
      
       
      
     




     

      
       
       
Here we say review banner but in the "Ship It!" tip we say Review menu. We should standardize on one …

      		
      
       maubinmaubin
      
      
       
      
     




     

      
       
       
Lets take out the word "providing" on this line to shorten the sentence a bit.

      		
      
       maubinmaubin
      
      
       
      
     




     

      
       
       
What's this addressing? Do other icons need it? Since we reuse this offset in &__close, can we add a constant …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Will this tip show up outside of the review dialog? Should we give it a name that gives it context …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Let's pull out the full manual URL to a variable, so that localized strings don't hard-code any parts of this …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
We may want to add role="button" to help screen readers know this is an action and not something that'll take …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This needs to preventDefault() and stopPropagation().

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This is the inverse of the hide action. Maybe generalize this into a _toggleTips() and have both the event and …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Not sure how the <p> and <span> relate here. The <p> is self-closing, but the <span> indents.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Can you add a Structure: to this and also include this in the main element's Structure:?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This should still have a Structure, even if it's a simple standalone already shown elsewhere. All parts of all CSS …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 



 

  

   




david




   

   

  

 



 















 





 

  

   




maubin




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    It actually fits quite nicely at the top of the dialog.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We should probably change all of the ’ apostrophes to the ' ones that we normally use. And any “ quotes to ". I'm guessing the other ones showed up just from copying text from Slack.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Here we say review banner but in the "Ship It!" tip we say Review menu. We should standardize on one (probably Review menu).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Lets take out the word "providing" on this line to shorten the sentence a bit.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




maubin




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    These tips are great!
    

    Change overall looks great, too. A few fairly small things.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/css/ui/alert.less
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    What's this addressing? Do other icons need it?
    

    Since we reuse this offset in &__close, can we add a constant or combine those two rules?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This was a mistake. I was intending to add this only to __close and it just ended up in the wrong spot.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Will this tip show up outside of the review dialog? Should we give it a name that gives it context for the review dialog, in case we want to add tips to the review request page or something down the road?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Let's pull out the full manual URL to a variable, so that localized strings don't hard-code any parts of this URL.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We may want to add role="button" to help screen readers know this is an action and not something that'll take them elsewhere.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This needs to preventDefault() and stopPropagation().
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This is the inverse of the hide action. Maybe generalize this into a _toggleTips() and have both the event and an events entry for this tie into that?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I actually meant to delete the show/hide calls here since that's what #updateTipsVisibility does.
      
 
      

      2. 


 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    One small docs thing.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/css/ui/alert.less
      


       (Diff revision 5)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Can you add a Structure: to this and also include this in the main element's Structure:?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This is always just an empty <span> but I'll add it to the main structure.
      
 
      

      2. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/css/ui/alert.less
      


       (Diff revisions 5 - 6)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Not sure how the <p> and <span> relate here. The <p> is self-closing, but the <span> indents.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/css/ui/alert.less
      


       (Diff revision 7)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This should still have a Structure, even if it's a simple standalone already shown elsewhere. All parts of all CSS components should have this.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      But there's no structure. It's just an empty <span> element with no children.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      The structure contains the tag the part applies to and any applicable children. There's zero in this case, but we still document the tag itself in all other cases.
      

      And actually, in this case, it catches something. A <span> isn't right for a button. We need this to be either a <button> itself, or use role="button" aria-label="..." tabindex="0", to be accessible. The docs should describe and show that.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      (Those should also be listed in "DOM Attributes").
      
 
      

      4. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-6.x (14ab0c8)





 


 










  

 









     

    

   

  

  

   
   Loading...