Fish Trophy

wenqin got a fish trophy!

add tips feature for reviewboard motd extension

Review Request #10001 — Created June 6, 2018 and updated

wenqin
rb-extension-pack
master
8d8c3f7...
rb-extension-pack
solarmist

This change makes motd (message of the day) extension more useful by providing with a tips feature. Admin can manually add tips, or uploading a text file with tips, and it would display a random tip below the message of the day banner.

Tested in local environment

  • 0
  • 0
  • 62
  • 1
  • 63
Description From Last Updated
Checks run (1 failed, 1 failed with error)
flake8 failed.
JSHint internal error.

flake8

wenqin
Review request changed

Checks run (1 failed, 1 failed with error)

flake8 failed.
JSHint internal error.

flake8

wenqin
wenqin
solarmist
  1. 
      
  2. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     

    tips_file

  3. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     

    Probably don't want to allow empty files.

  4. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     

    Load tips file. One line per tip. If using HTML it must be properly escaped.

  5. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     

    tips_file

  6. This is mostly a copy paste extension of the existing list_edit_widget, but the code lives in Djblets so it needed to be duplicated here.

  7. I think this is all you need

    class ListEditWidgetWithClearButton(ListEditWidget):
        template_name = 'rbmotd/list_edit_widget_with_clear_button.html'
    
  8. rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     

    Not needed.

  9. Are there any changes to this? It doesn't seems like it, but the code has changed.

    https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

    1. https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py
      The code copied from this file

  10. rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Not needed.

  11. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     

    Look into less. I think this can re-use most of

    #motd-close #motd {
    ...
    
    }
    
    #motd-close {
      background: @bg-color-tip;
    }
    
  12. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     

    Just add this to the existing one.

  13. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     

    Just add this to the existing one.

  14. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Indentation should be 4 spaces.

  15. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Please add a comment block like
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  16. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment block.

    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  17. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Not needed.

  18. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment why are you seperating this?

  19. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Not needed.

  20. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Unneeded blank.

  21. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  22. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
  23. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  24. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     

    blank line between these

  25. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  26. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     

    Blank line

  27. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Instead of a for loop use:

    tips.forEach{}
    

    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

  28. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    https://api.jquery.com/last/

  29. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     

    Use a variable to store this then manipulate it.

  30. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    Comment block.

  31. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     

    You can filter better so you don't need to use a for loop. Then use $.each(function(){$this.click()}.

    https://stackoverflow.com/questions/10651349/how-to-select-empty-inputs-value-using-jquery?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

    1. Since we are filtering on '.list-edit-entry input' but trigger events on '.list-edit-remove-item', we need an index to keep trakc of which entry we are at right now

  32. This is also a copy paste from Djblets.

  33. This is the new code.

  34. rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    4 spaces for JS code.

  35. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can we just make this two functions where we pass in {{motd_close_value}} or the new one and '#modt' or '#motd-tip', etc.

  36. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     

    Make this a small block along with the motd one and move it to the top and keep the JS code at the bottom.

  37. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    4 spaces for JS code.

  38. I'd make this part of the code that checks the cookie.

    1. This is when the tips are enabled, but admin enters nothing in the tips field. We want to prevent empty tip showing up

  39. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     

    Unneeded new lines.

  40. Comment block

  41. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     

    Shouldn't be needed if using .forEach

  42. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     

    I feel like this could be cleaner, but I'm not positive, but otherwise please add a comment about why this is needed.

  43. Comment block.

    Mention that this is to handle windows newlines.

    1. what is a windows newlines?

    2. Windows uses '/r/n' as the new line character. Linux and Mac systems use only '/n' as their new line character.

  44. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     

    Extra whitespace? Look at the original files and try to match the style that's already there.

  45. 
      
wenqin
  1. 
      
  2. There are no changes on this function. I copied the codes over from ListEditWidget class from this file:
    https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

  3. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     

    I tried before and that did not work.

    I think the only two way to reuse a component is
    (1) define a mixing and pass in variables
    (2) extension.

  4. 
      
wenqin
Review request changed

Checks run (1 failed, 1 failed with error)

flake8 failed.
JSHint internal error.

flake8

solarmist
  1. Looks good to me.

  2. rbmotd/rbmotd/forms.py (Diff revision 4)
     
     

    Try this.

    help_text=_('Load tips file. One line per tip. '
                'This field expects valid text file. '
                'If using HTML it must be properly escaped.')
    
  3. rbmotd/rbmotd/forms.py (Diff revision 4)
     
     

    I think I'd make some variables for message, tips and tips_file. That'll take care of the lines too long.

  4. If this doesn't have any code changes it isn't needed, it'll just re-use the method from ListEditWidget.

  5. rbmotd/rbmotd/static/css/motd.less (Diff revision 4)
     
     

    Uppercase please.

  6. 
      
Loading...