Background Color Selector When Viewing Images

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

mderose
Review Board
master
reviewboard, students

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 Author
Background Color Selector When Viewing Images
mderose123
Added fixed HSHint raised
mderose123
Developed event handlers for changing the background color
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
mderose123
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
Small changes made to fix UI bug - nno resolution found yet
mderose123
Fixed JSHint error regarding missing semi-colon
mderose123
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
Addendum Added missing semi colon identified JSHint
mderose123
Addendum made coding style changes based off of Christian's recommendations
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
mderose123
Addendum removed straggler white spaces not caught in my previous commit
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
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
mderose123
Reordered some css properties to be in alphabetical order
mderose123
Added space between labels and backgroundMenuTemplate declaration
mderose123
Removed unnecessary code, changed pixel to em in less file and fixed coding style
mderose123
Fixed alignment of custom colour icon
mderose123
Addendum added missing semicolon
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 Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123

Diff:

Revision 3 (+348 -58)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mderose
mderose
mderose
mike_conley
  1. 
      
  2. 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 Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
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
mderose123
-
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123
+
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Small changes made to fix UI bug - nno resolution found yet
mderose123

Diff:

Revision 5 (+362 -62)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mderose
mderose
mderose
Review request changed

Commits:

Summary Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
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
mderose123
-
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
-
Small changes made to fix UI bug - nno resolution found yet
mderose123
-
Fixed JSHint error regarding missing semi-colon
mderose123
-
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123
+
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Small changes made to fix UI bug - nno resolution found yet
mderose123
+
Fixed JSHint error regarding missing semi-colon
mderose123
+
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123

Diff:

Revision 8 (+443 -165)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mderose
mderose
mderose
mderose
mderose
mderose
chipx86
  1. 
      
  2. 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)
     
     
     
     
     
     

    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. 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. This is too generic a name, so it'll be especially important to namespace this, as per our docs.

  6. Blank line between these.

  7. 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. Can you reorder these to be in alphabetical order?

  9. The blank line here should stay.

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

    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. Blank line between these.

  12. Space after the if, before the (.

  13. 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)
     
     
     
     
     
     
     
     
     
     
     
     

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

  15. 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. Always use double quotes for attribute values, and never self-close tags.

  17. You can use chaining to reduce the code here:

    $('<div class="....">')
        .append($backgroundMenu)
        .append($resolutionMenu)
        .appendTo($header);
    
  18. 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.

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

  20. Missing a period.

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

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

  22. 
      
mderose
Review request changed

Commits:

Summary Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
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
mderose123
-
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
-
Small changes made to fix UI bug - nno resolution found yet
mderose123
-
Fixed JSHint error regarding missing semi-colon
mderose123
-
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
-
Addendum Added missing semi colon identified JSHint
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123
+
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Small changes made to fix UI bug - nno resolution found yet
mderose123
+
Fixed JSHint error regarding missing semi-colon
mderose123
+
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
+
Addendum Added missing semi colon identified JSHint
mderose123
+
Addendum made coding style changes based off of Christian's recommendations
mderose123

Diff:

Revision 10 (+542 -264)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mderose
mderose
mderose
mderose
mderose
mderose
mderose
david
  1. 
      
  2. Please wrap your description and testing done sections to be 80 characters wide.

  3. 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. This should also use one of the #rb-ns-ui.colors definitions (see reviewboard/static/rb/css/ui/colors.less) instead of "grey"

  5. Please use single quotes instead of double.

  6. Please use single quotes instead of double.

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

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

    $('<div class="...">')
        .append(...)
    
  9. 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. 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. 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)
     
     
     
     
     
     
     
     
     
     
     

    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. A more standardized name (in our modern standard) would be -has-checkered-background.

  4. 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. Blank line between these.

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

  7. 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. No blank line here.

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

    Try:

    events: {
        ...
        'click .rb-o-image-background-blah-blah-blah':
            '_onBlahChanged',
        ...
    }
    
  10. When you're setting a single attribute, pass each in as a parameter:

    this.model.set('checkeredBackground', false);
    
  11. 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)
     
     
     
     
     
     

    This can now be:

    this.$el.toggleClass('-has-checkered-background',
                         isCheckeredBackground);
    
  13. All this should be 1-space indentation.

  14. 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. The /> must be a >.

  16. for must align with class.

  17. This must also be <%-.

  18. This is indented 1 space too few.

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

  20. Missing comma after this last line in the list.

  21. This could be one statement.

  22. This could be one statement.

  23. 
      
chipx86
  1. 
      
  2. 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 Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
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
mderose123
-
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
-
Small changes made to fix UI bug - nno resolution found yet
mderose123
-
Fixed JSHint error regarding missing semi-colon
mderose123
-
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
-
Addendum Added missing semi colon identified JSHint
mderose123
-
Addendum made coding style changes based off of Christian's recommendations
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
mderose123
-
Addendum removed straggler white spaces not caught in my previous commit
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
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
mderose123
-
Reordered some css properties to be in alphabetical order
mderose123
-
Added space between labels and backgroundMenuTemplate declaration
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123
+
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Small changes made to fix UI bug - nno resolution found yet
mderose123
+
Fixed JSHint error regarding missing semi-colon
mderose123
+
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
+
Addendum Added missing semi colon identified JSHint
mderose123
+
Addendum made coding style changes based off of Christian's recommendations
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
mderose123
+
Addendum removed straggler white spaces not caught in my previous commit
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
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
mderose123
+
Reordered some css properties to be in alphabetical order
mderose123
+
Added space between labels and backgroundMenuTemplate declaration
mderose123
+
Removed unnecessary code, changed pixel to em in less file and fixed coding style
mderose123

Diff:

Revision 16 (+647 -369)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

mderose
Review request changed

Commits:

Summary Author
-
Background Color Selector When Viewing Images
mderose123
-
Added fixed HSHint raised
mderose123
-
Developed event handlers for changing the background color
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
mderose123
-
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
-
Small changes made to fix UI bug - nno resolution found yet
mderose123
-
Fixed JSHint error regarding missing semi-colon
mderose123
-
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
-
Addendum Added missing semi colon identified JSHint
mderose123
-
Addendum made coding style changes based off of Christian's recommendations
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
mderose123
-
Addendum removed straggler white spaces not caught in my previous commit
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
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
mderose123
-
Reordered some css properties to be in alphabetical order
mderose123
-
Added space between labels and backgroundMenuTemplate declaration
mderose123
-
Removed unnecessary code, changed pixel to em in less file and fixed coding style
mderose123
+
Background Color Selector When Viewing Images
mderose123
+
Added fixed HSHint raised
mderose123
+
Developed event handlers for changing the background color
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
mderose123
+
Addendum added missing semicolon as per ReviewBot's recommendation
mderose123
+
Small changes made to fix UI bug - nno resolution found yet
mderose123
+
Fixed JSHint error regarding missing semi-colon
mderose123
+
Added css properties to the image-review-ui.less file as per Mike Conley's recommendation
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.
mderose123
+
Addendum Added missing semi colon identified JSHint
mderose123
+
Addendum made coding style changes based off of Christian's recommendations
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
mderose123
+
Addendum removed straggler white spaces not caught in my previous commit
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
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
mderose123
+
Reordered some css properties to be in alphabetical order
mderose123
+
Added space between labels and backgroundMenuTemplate declaration
mderose123
+
Removed unnecessary code, changed pixel to em in less file and fixed coding style
mderose123
+
Fixed alignment of custom colour icon
mderose123
+
Addendum added missing semicolon
mderose123

Diff:

Revision 17 (+652 -370)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...