• 
      

    Background Color Selector When Viewing Images

    Review Request #11445 — Created Feb. 14, 2021 and updated

    Information

    Review Board
    master

    Reviewers

    This will allow users to select the colour outside of the image to help
    improve visibility. For instance if an image is mostlywhite and the background
    is white it may be difficult tounderstand what the image is. The background
    colour selector menu is present on all image review modes (i.e. single
    revision, "no diff", two-up, difference, split, and onion skin). The
    background colour selector and the image resolution menu have now been moved
    to a line below the caption in the image reviewer in order to better control
    the arrangements of all elements in the header when the screen is adjusted to
    a smaller size.

    Manual testing has been done to ensure that the background color of the image
    review page can change to white, black, grey/black checkerboard, or a custom
    colour for all image views (i.e. single revision, "no diff", two-up, difference,
    split, and onion skin). This has been tested on Mozilla Firefox, Safari and
    Google Chrome and the menu appears as intended.

    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    f7188254230e300284e8085f30c177bda2664905 mderose123
    Added fixed HSHint raised
    a4f46881c26a25a75417cf57716c36e172bc010c mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123
    Small changes made to fix UI bug - nno resolution found yet
    9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123
    Fixed JSHint error regarding missing semi-colon
    39fb276e93c847e4319821eb9d348cf549232563 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    39a633b49bff00a1da86beac9691733449c19a14 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123
    Addendum Added missing semi colon identified JSHint
    bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123
    Addendum made small code styling changes based off of the recommendations from the JSHint in addition to removing extraneous white space throughout all changed files
    33d6f50ceb97cb2b4d466be9060341070539936c mderose123
    Addendum removed straggler white spaces not caught in my previous commit
    01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123
    Addendendum changed process of changing the background surrounding an image to a grey black checkerboard by adding and removing a modifier from the image-review-ui class
    6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123
    Added css variable for grey colour used in image-review-ui.less, replaced unnecessary double quotations with single quotations and removed redundant variable name as per David's recommendations
    5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123
    Reordered some css properties to be in alphabetical order
    ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123
    Added space between labels and backgroundMenuTemplate declaration
    2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
    Removed unnecessary code, changed pixel to em in less file and fixed coding style
    23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123
    Fixed alignment of custom colour icon
    e61b0278b85a765d2749ad505de93fa90db3503c mderose123
    Addendum added missing semicolon
    4f1dd30545cf9ae229884ec81b7efea762c465c4 mderose123

    Description From Last Updated

    You're working with some older code that predates our modern CSS naming conventions, but it'd still be good to make …

    chipx86chipx86

    Please wrap your description and testing done sections to be 80 characters wide.

    daviddavid

    I think, personally, this UI fits in really nicely with Review Board's pre-existing metaphors. Great job! A few notes: We …

    mike_conleymike_conley

    I think we should have the captions be last. We want to think of the UI in logical components. We …

    chipx86chipx86

    Col: 76 Missing semicolon.

    reviewbotreviewbot

    Col: 97 Missing semicolon.

    reviewbotreviewbot

    Col: 45 Missing semicolon.

    reviewbotreviewbot

    Col: 67 Missing semicolon.

    reviewbotreviewbot

    The text-align should predecede the nested table rule, because it's relevant to .review-ui-header. You'd also want a blank line between …

    chipx86chipx86

    Can you put this in alphabetical order? While not always appropriate for all CSS rules, it's a good point to …

    chipx86chipx86

    This is too generic a name, so it'll be especially important to namespace this, as per our docs.

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Can you do testing on Firefox and Safari for these?

    chipx86chipx86

    Can you reorder these to be in alphabetical order?

    chipx86chipx86

    The blank line here should stay.

    chipx86chipx86

    Some important comments here: Keep line lengths in mind. Lines cannot go beyond 79 characters. These sorts of CSS changes …

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    Space after the if, before the (.

    chipx86chipx86

    Can you go through the file and make sure there's no extra whitespace (identified by these red blocks)?

    chipx86chipx86

    This would be best done as a _.template somewhere, using _.dedent (see other parts of the codebase for examples).

    chipx86chipx86

    Any text must be localized and provided in. This is easier done with templates. You can localize a string using: …

    chipx86chipx86

    Always use double quotes for attribute values, and never self-close tags.

    chipx86chipx86

    </> isn't valid.

    chipx86chipx86

    You can use chaining to reduce the code here: $('<div class="....">') .append($backgroundMenu) .append($resolutionMenu) .appendTo($header);

    chipx86chipx86

    You can set multiple model attributes at once: this.model.set({ checkeredBackground: false, background: color, }); This helps with ensuring that signal …

    chipx86chipx86

    This can be one statement. Also, code should always use single quotes instead of double quotes, unless the text in …

    chipx86chipx86

    Missing a period. This must also be one line (and can't go beyond the line length).

    chipx86chipx86

    This should probably be driven by the model. The model should listen for a change to checkeredBackground. If set, it …

    chipx86chipx86

    Col: 12 Unexpected ')'.

    reviewbotreviewbot

    Col: 12 Expected an identifier and instead saw ')'.

    reviewbotreviewbot

    Col: 13 Expected ')' and instead saw ';'.

    reviewbotreviewbot

    Col: 14 Missing semicolon.

    reviewbotreviewbot

    Col: 15 '$toolBar' is defined but never used.

    reviewbotreviewbot

    Let's use #rb-ns-ui.colors[@grey-40] instead of #808080

    daviddavid

    This should also use one of the #rb-ns-ui.colors definitions (see reviewboard/static/rb/css/ui/colors.less) instead of "grey"

    daviddavid

    Please use single quotes instead of double.

    daviddavid

    Please use single quotes instead of double.

    daviddavid

    This should probably be called backgroundMenuTemplate. The $ prefix is for jQuery objects.

    daviddavid

    We could skip defining this as a variable and just do: $('<div class="...">') .append(...)

    daviddavid

    Let's wrap this like so: .append(backgroundMenuTemplate({ backgroundLabel: backgroundLabel, customLabel: customLabel, }) .append($resolutionMenu) ... Note that we also don't need quotes …

    daviddavid

    Please add a trailing comma at the end of this.

    daviddavid

    I only see one place each where we use these. We don't need to define new variables for them. Let's …

    chipx86chipx86

    A more standardized name (in our modern standard) would be -has-checkered-background.

    chipx86chipx86

    Do we need !important?

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    We usually make display the first, or one of the first, properties in a rule. Can you move it up?

    chipx86chipx86

    Where possible, let's use em units. These are going to scale better than pixel units, making them more accessible for …

    chipx86chipx86

    No blank line here.

    chipx86chipx86

    These are looking pretty long. Not uncommon for this to happen. Try: events: { ... 'click .rb-o-image-background-blah-blah-blah': '_onBlahChanged', ... }

    chipx86chipx86

    When you're setting a single attribute, pass each in as a parameter: this.model.set('checkeredBackground', false);

    chipx86chipx86

    this.$el is the .image-review-ui component, so rather than scanning the entire DOM for it each time, you can just use …

    chipx86chipx86

    This can now be: this.$el.toggleClass('-has-checkered-background', isCheckeredBackground);

    chipx86chipx86

    All this should be 1-space indentation.

    chipx86chipx86

    Only one space indentation. This should also be <%- backgroundLabel %>, in case the text gets localized to something that …

    chipx86chipx86

    The /> must be a >.

    chipx86chipx86

    for must align with class.

    chipx86chipx86

    This must also be <%-.

    chipx86chipx86

    This is indented 1 space too few.

    chipx86chipx86

    Rather than defining the variables above, you can pass in the strings here directly.

    chipx86chipx86

    Missing comma after this last line in the list.

    chipx86chipx86

    This could be one statement.

    chipx86chipx86

    This could be one statement.

    chipx86chipx86

    Doc summaries must be a single line. You can go into more detail in the description. I'd just say "changed …

    chipx86chipx86

    Col: 31 Missing semicolon.

    reviewbotreviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    mderose
    mderose
    mderose
    mderose
    Review request changed
    Testing Done:
      +

    Manual testing has been done to ensure that the background color of the image review page can change to white, black, grey/black checkerboard, or a custom colour for all image views.

    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    mderose
    mderose
    mike_conley
    1. 
        
    2. Show all issues

      I think, personally, this UI fits in really nicely with Review Board's pre-existing metaphors. Great job!

      A few notes:

      1. We probably still want the filename in the center of this, rather than pushed to the right.
      2. The labels should be vertical-center aligned
      3. Either both should end with : or not.
      4. We probably want a little extra space between "Custom" and the image on its left - maybe the same amount of distance between the background color selectors.
    3. 
        
    mderose
    Review request changed
    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Small changes made to fix UI bug - nno resolution found yet
    ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    mderose
    mderose
    Review request changed
    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Small changes made to fix UI bug - nno resolution found yet
    ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123
    Fixed JSHint error regarding missing semi-colon
    71105512c62675d89fafb584902af4933f5eaad7 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Small changes made to fix UI bug - nno resolution found yet
    ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123
    Fixed JSHint error regarding missing semi-colon
    71105512c62675d89fafb584902af4933f5eaad7 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123
    Removed Files:
    Added Files:

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    mderose
    mderose
    mderose
    mderose
    mderose
    chipx86
    1. 
        
    2. Show all issues

      You're working with some older code that predates our modern CSS naming conventions, but it'd still be good to make use of them for all new class names. See our documentation on that.

    3. reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 9)
       
       
       
       
       
       
      Show all issues

      The text-align should predecede the nested table rule, because it's relevant to .review-ui-header.

      You'd also want a blank line between that and nested rules.

    4. Show all issues

      Can you put this in alphabetical order? While not always appropriate for all CSS rules, it's a good point to start. The exception to it is that we group related properties (width/height, position/top/right/bottom/left, etc.) together.

      This applies to other rules below.

    5. Show all issues

      This is too generic a name, so it'll be especially important to namespace this, as per our docs.

    6. Show all issues

      Blank line between these.

    7. Show all issues

      Can you do testing on Firefox and Safari for these?

      1. I've added screenshots of how the custom colour selector looks for Mozilla and Safari

    8. Show all issues

      Can you reorder these to be in alphabetical order?

    9. Show all issues

      The blank line here should stay.

    10. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 9)
       
       
       
       
       
       
      Show all issues

      Some important comments here:

      1. Keep line lengths in mind. Lines cannot go beyond 79 characters.
      2. These sorts of CSS changes are best done in CSS, not in JavaScript.
      3. Always use const or let instead of var.
      1. May I ask you to provide more specific details for #2. I've created a variable in the corresponding .less file for the checkerboard pattern on a larger background and I can't seem to target it in the view file. I'm not sure if this is something that is possible using jquery. If I misinterpretted your recommendation would you mind providing more specific implementation details of how to accomplish this. Sorry for the trouble.

    11. Show all issues

      Blank line between these.

    12. Show all issues

      Space after the if, before the (.

    13. Show all issues

      Can you go through the file and make sure there's no extra whitespace (identified by these red blocks)?

    14. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 9)
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      This would be best done as a _.template somewhere, using _.dedent (see other parts of the codebase for examples).

    15. Show all issues

      Any text must be localized and provided in. This is easier done with templates. You can localize a string using:

      _`Background`
      
      1. I've made an attempt to localize the strings for the Background and Custom label in my recent commits. Please let me know if I implemented this correctly.

    16. Show all issues

      Always use double quotes for attribute values, and never self-close tags.

    17. Show all issues

      </> isn't valid.

    18. Show all issues

      You can use chaining to reduce the code here:

      $('<div class="....">')
          .append($backgroundMenu)
          .append($resolutionMenu)
          .appendTo($header);
      
    19. Show all issues

      You can set multiple model attributes at once:

      this.model.set({
          checkeredBackground: false,
          background: color,
      });
      

      This helps with ensuring that signal handlers can get the new set of attributes at the same time, rather than triggering a bunch of different signals and leaving it up to the code to sort it out.

      While not an issue necessarily right here, it's a pattern that's good to adopt for Backbone.

    20. Show all issues

      This can be one statement.

      Also, code should always use single quotes instead of double quotes, unless the text in the string requires a single quote.

      These apply to code below.

    21. Show all issues

      Missing a period.

      This must also be one line (and can't go beyond the line length).

    22. Show all issues

      This should probably be driven by the model.

      The model should listen for a change to checkeredBackground. If set, it should set the background to a value.

      It should probably also go the other way. If explicitly setting a background, unset checkeredBackground. That would simplify code above.

    23. 
        
    mderose
    Review request changed
    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Small changes made to fix UI bug - nno resolution found yet
    ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123
    Fixed JSHint error regarding missing semi-colon
    71105512c62675d89fafb584902af4933f5eaad7 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123
    Addendum Added missing semi colon identified JSHint
    310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    5221912914ed8de44527040095b1ea1005ec18d5 mderose123
    Added fixed HSHint raised
    c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123
    Small changes made to fix UI bug - nno resolution found yet
    ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123
    Fixed JSHint error regarding missing semi-colon
    71105512c62675d89fafb584902af4933f5eaad7 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123
    Addendum Added missing semi colon identified JSHint
    310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    928aae64a23a4682c40c732929465d68b7700c7f mderose123

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    mderose
    mderose
    mderose
    mderose
    mderose
    mderose
    david
    1. 
        
    2. Show all issues

      Please wrap your description and testing done sections to be 80 characters wide.

    3. Show all issues

      Let's use #rb-ns-ui.colors[@grey-40] instead of #808080

      1. I had to create a variable to store this value since I wasn't able to add the !important property if I replaced #808080 with #rb-ns-ui.colors[@grey-40]

    4. Show all issues

      This should also use one of the #rb-ns-ui.colors definitions (see reviewboard/static/rb/css/ui/colors.less) instead of "grey"

    5. Show all issues

      Please use single quotes instead of double.

    6. Show all issues

      Please use single quotes instead of double.

    7. Show all issues

      This should probably be called backgroundMenuTemplate. The $ prefix is for jQuery objects.

    8. Show all issues

      We could skip defining this as a variable and just do:

      $('<div class="...">')
          .append(...)
      
    9. Show all issues

      Let's wrap this like so:

      .append(backgroundMenuTemplate({
          backgroundLabel: backgroundLabel,
          customLabel: customLabel,
      })
      .append($resolutionMenu)
      ...
      

      Note that we also don't need quotes around the keys that we're passing in there.

    10. Show all issues

      Please add a trailing comma at the end of this.

    11. 
        
    david
    1. 
        
    2. Please also go through and mark "Fixed" on any old issues which have been fixed up.

    3. 
        
    mderose
    mderose
    CL
    1. Hi Matthew! It is an awesome feature that you implement! 😄 I was wondering why in the styling sheets, sometimes you are using the tag identifier started with the & sign, such as &.-black and &.-white, instead of the typical .-white? Also, for the checkered background selection, why do we need to specify the background-position and background-size, while the other selections don't?

    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      I think we should have the captions be last.

      We want to think of the UI in logical components. We have here, in order:

      1. Controls for manipulating the view
      2. Labels that describe the content below
      3. Controls for manipulating the view
      4. Content

      It'd be much nicer as:

      1. Controls for manipulating the view
      2. Labels that describe the content below
      3. Content
    3. 
        
    chipx86
    1. 
        
    2. reviewboard/static/rb/css/pages/image-review-ui.less (Diff revision 15)
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      I only see one place each where we use these. We don't need to define new variables for them. Let's put them where they're needed.

    3. Show all issues

      A more standardized name (in our modern standard) would be -has-checkered-background.

    4. Show all issues

      Do we need !important?

      1. Yes because when I didn't have the !important when I switched from one background colour option to the checkerboard the black checkerboard would appear but the background was the previous background colour applied and not grey. This was the only solution I could find.

    5. Show all issues

      Blank line between these.

    6. Show all issues

      We usually make display the first, or one of the first, properties in a rule. Can you move it up?

    7. Show all issues

      Where possible, let's use em units. These are going to scale better than pixel units, making them more accessible for users.

      Usually we have units in increments of 0.5em, depending on what we're displaying (e.g., 1em, 2em, 1.5em, etc.). Not a hard-and-fast rule, but that does help keep a visual consistency.

      Same throughout the change.

      1. I tried my best to follow this however for the custom colour icon I really struggled to find a solution that was a multiple of 0.5. Lines 314 annd 315 of the image-review-ui.less file were the two properties I couldn't avoid using a value that was not a multiple of 0.5em

    8. Show all issues

      No blank line here.

    9. Show all issues

      These are looking pretty long. Not uncommon for this to happen.

      Try:

      events: {
          ...
          'click .rb-o-image-background-blah-blah-blah':
              '_onBlahChanged',
          ...
      }
      
    10. Show all issues

      When you're setting a single attribute, pass each in as a parameter:

      this.model.set('checkeredBackground', false);
      
    11. Show all issues

      this.$el is the .image-review-ui component, so rather than scanning the entire DOM for it each time, you can just use this.$el.

    12. reviewboard/static/rb/js/views/imageReviewableView.es6.js (Diff revision 15)
       
       
       
       
       
       
      Show all issues

      This can now be:

      this.$el.toggleClass('-has-checkered-background',
                           isCheckeredBackground);
      
    13. Show all issues

      All this should be 1-space indentation.

    14. Show all issues

      Only one space indentation.

      This should also be <%- backgroundLabel %>, in case the text gets localized to something that isn't HTML-safe (<%- will escape text).

    15. Show all issues

      The /> must be a >.

    16. Show all issues

      for must align with class.

    17. Show all issues

      This must also be <%-.

    18. Show all issues

      This is indented 1 space too few.

    19. Show all issues

      Rather than defining the variables above, you can pass in the strings here directly.

    20. Show all issues

      Missing comma after this last line in the list.

    21. Show all issues

      This could be one statement.

    22. Show all issues

      This could be one statement.

    23. 
        
    chipx86
    1. 
        
    2. Show all issues

      Doc summaries must be a single line.

      You can go into more detail in the description.

      I'd just say "changed to a checkered pattern."

    3. 
        
    mderose
    Review request changed
    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    f7188254230e300284e8085f30c177bda2664905 mderose123
    Added fixed HSHint raised
    a4f46881c26a25a75417cf57716c36e172bc010c mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123
    Small changes made to fix UI bug - nno resolution found yet
    9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123
    Fixed JSHint error regarding missing semi-colon
    39fb276e93c847e4319821eb9d348cf549232563 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    39a633b49bff00a1da86beac9691733449c19a14 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123
    Addendum Added missing semi colon identified JSHint
    bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123
    Addendum made small code styling changes based off of the recommendations from the JSHint in addition to removing extraneous white space throughout all changed files
    33d6f50ceb97cb2b4d466be9060341070539936c mderose123
    Addendum removed straggler white spaces not caught in my previous commit
    01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123
    Addendendum changed process of changing the background surrounding an image to a grey black checkerboard by adding and removing a modifier from the image-review-ui class
    6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123
    Added css variable for grey colour used in image-review-ui.less, replaced unnecessary double quotations with single quotations and removed redundant variable name as per David's recommendations
    5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123
    Reordered some css properties to be in alphabetical order
    ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123
    Added space between labels and backgroundMenuTemplate declaration
    2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    f7188254230e300284e8085f30c177bda2664905 mderose123
    Added fixed HSHint raised
    a4f46881c26a25a75417cf57716c36e172bc010c mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123
    Small changes made to fix UI bug - nno resolution found yet
    9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123
    Fixed JSHint error regarding missing semi-colon
    39fb276e93c847e4319821eb9d348cf549232563 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    39a633b49bff00a1da86beac9691733449c19a14 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123
    Addendum Added missing semi colon identified JSHint
    bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123
    Addendum made small code styling changes based off of the recommendations from the JSHint in addition to removing extraneous white space throughout all changed files
    33d6f50ceb97cb2b4d466be9060341070539936c mderose123
    Addendum removed straggler white spaces not caught in my previous commit
    01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123
    Addendendum changed process of changing the background surrounding an image to a grey black checkerboard by adding and removing a modifier from the image-review-ui class
    6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123
    Added css variable for grey colour used in image-review-ui.less, replaced unnecessary double quotations with single quotations and removed redundant variable name as per David's recommendations
    5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123
    Reordered some css properties to be in alphabetical order
    ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123
    Added space between labels and backgroundMenuTemplate declaration
    2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
    Removed unnecessary code, changed pixel to em in less file and fixed coding style
    23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    mderose
    Review request changed
    Commits:
    Summary ID Author
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    f7188254230e300284e8085f30c177bda2664905 mderose123
    Added fixed HSHint raised
    a4f46881c26a25a75417cf57716c36e172bc010c mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123
    Small changes made to fix UI bug - nno resolution found yet
    9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123
    Fixed JSHint error regarding missing semi-colon
    39fb276e93c847e4319821eb9d348cf549232563 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    39a633b49bff00a1da86beac9691733449c19a14 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123
    Addendum Added missing semi colon identified JSHint
    bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123
    Addendum made small code styling changes based off of the recommendations from the JSHint in addition to removing extraneous white space throughout all changed files
    33d6f50ceb97cb2b4d466be9060341070539936c mderose123
    Addendum removed straggler white spaces not caught in my previous commit
    01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123
    Addendendum changed process of changing the background surrounding an image to a grey black checkerboard by adding and removing a modifier from the image-review-ui class
    6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123
    Added css variable for grey colour used in image-review-ui.less, replaced unnecessary double quotations with single quotations and removed redundant variable name as per David's recommendations
    5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123
    Reordered some css properties to be in alphabetical order
    ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123
    Added space between labels and backgroundMenuTemplate declaration
    2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
    Removed unnecessary code, changed pixel to em in less file and fixed coding style
    23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123
    Background Color Selector When Viewing Images
    This will allow users to select the colour outside of the image to help improve visibility. For instance if an image is mostly white and the background is white it may be difficult to understand what the image is.
    f7188254230e300284e8085f30c177bda2664905 mderose123
    Added fixed HSHint raised
    a4f46881c26a25a75417cf57716c36e172bc010c mderose123
    Developed event handlers for changing the background color
    by adding a background field in the imageReviewableModel and creating three events in the imageReviewableView to change the background based on what option the user selects. Also matched css class names to better correspond with the classes used in the menu used for changing the zoomed scale of an image. Also removed a redundant css class created in a previous commit
    d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123
    Implemented the functionality to change the background color selector for the image review window for all image views. Currently the user is able to choose a white, black, black/grey checkerboard, or a custom color as a background for an image
    583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123
    Addendum added missing semicolon as per ReviewBot's recommendation
    8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123
    Small changes made to fix UI bug - nno resolution found yet
    9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123
    Fixed JSHint error regarding missing semi-colon
    39fb276e93c847e4319821eb9d348cf549232563 mderose123
    Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
    39a633b49bff00a1da86beac9691733449c19a14 mderose123
    The background colour selector and the image resolution menu have now been moved to a line below the caption in the image reviewer in order to better control the arrangements of all elements in the header when the screen is adjusted to a smaller size. In addition to this the custom colour background option will now change the background colour when the icon is selected and when a colour change is detected in the colour input menu.
    48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123
    Addendum Added missing semi colon identified JSHint
    bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123
    Addendum made coding style changes based off of Christian's recommendations
    2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123
    Addendum made small code styling changes based off of the recommendations from the JSHint in addition to removing extraneous white space throughout all changed files
    33d6f50ceb97cb2b4d466be9060341070539936c mderose123
    Addendum removed straggler white spaces not caught in my previous commit
    01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123
    Addendendum changed process of changing the background surrounding an image to a grey black checkerboard by adding and removing a modifier from the image-review-ui class
    6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123
    Added css variable for grey colour used in image-review-ui.less, replaced unnecessary double quotations with single quotations and removed redundant variable name as per David's recommendations
    5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123
    Reordered some css properties to be in alphabetical order
    ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123
    Added space between labels and backgroundMenuTemplate declaration
    2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
    Removed unnecessary code, changed pixel to em in less file and fixed coding style
    23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123
    Fixed alignment of custom colour icon
    e61b0278b85a765d2749ad505de93fa90db3503c mderose123
    Addendum added missing semicolon
    4f1dd30545cf9ae229884ec81b7efea762c465c4 mderose123

    Checks run (2 succeeded)

    flake8 passed.
    JSHint passed.