Fish Trophy

wenqin got a fish trophy!

add tips feature for reviewboard motd extension

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

Information

rb-extension-pack
master

Reviewers

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

Description From Last Updated

E203 whitespace before ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'django.utils.six.moves.range' imported but unused

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

tips_file

solarmistsolarmist

Probably don't want to allow empty files.

solarmistsolarmist

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

solarmistsolarmist

tips_file

solarmistsolarmist

I think this is all you need class ListEditWidgetWithClearButton(ListEditWidget): template_name = 'rbmotd/list_edit_widget_with_clear_button.html'

solarmistsolarmist

Not needed.

solarmistsolarmist

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

solarmistsolarmist

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

wenqinwenqin

Not needed.

solarmistsolarmist

Look into less. I think this can re-use most of #motd-close #motd { ... } #motd-close { background: @bg-color-tip; }

solarmistsolarmist

I tried before and that did not work. I think the only two way to reuse a component is (1) …

wenqinwenqin

Just add this to the existing one.

solarmistsolarmist

Just add this to the existing one.

solarmistsolarmist

Indentation should be 4 spaces.

solarmistsolarmist

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

solarmistsolarmist

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

solarmistsolarmist

Not needed.

solarmistsolarmist

Comment why are you seperating this?

solarmistsolarmist

Not needed.

solarmistsolarmist

Unneeded blank.

solarmistsolarmist

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

solarmistsolarmist

Ditto.

solarmistsolarmist

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

solarmistsolarmist

blank line between these

solarmistsolarmist

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

solarmistsolarmist

Blank line

solarmistsolarmist

Instead of a for loop use: tips.forEach{} https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

solarmistsolarmist

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

solarmistsolarmist

Use a variable to store this then manipulate it.

solarmistsolarmist

Comment block.

solarmistsolarmist

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

solarmistsolarmist

4 spaces for JS code.

solarmistsolarmist

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

solarmistsolarmist

Make this a small block along with the motd one and move it to the top and keep the JS …

solarmistsolarmist

4 spaces for JS code.

solarmistsolarmist

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

solarmistsolarmist

Unneeded new lines.

solarmistsolarmist

Comment block

solarmistsolarmist

Shouldn't be needed if using .forEach

solarmistsolarmist

.forEach

solarmistsolarmist

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

solarmistsolarmist

Comment block. Mention that this is to handle windows newlines.

solarmistsolarmist

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

solarmistsolarmist

E501 line too long (136 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

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

solarmistsolarmist
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. 
      
david
Review request changed

Status: Discarded

Loading...