Background Color Selector When Viewing Images
Review Request #11445 — Created Feb. 14, 2021 and updated
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 |
---|---|---|
f7188254230e300284e8085f30c177bda2664905 | mderose123 | |
a4f46881c26a25a75417cf57716c36e172bc010c | mderose123 | |
d1d129a1d3f5b4c780331a29f890cb1dc22728ee | mderose123 | |
583727ca00da0095be8b3b244a08c68a9ecf2921 | mderose123 | |
8c99f19399f2b0b7dfb94bb4c959c8db5c72523c | mderose123 | |
9a0dd0e977d97ad7702ea5cc5eb517d803576304 | mderose123 | |
39fb276e93c847e4319821eb9d348cf549232563 | mderose123 | |
39a633b49bff00a1da86beac9691733449c19a14 | mderose123 | |
48fb7fe485a5fc74c7a3f8630b830fafaa8651ee | mderose123 | |
bee7caae3cecf3f88df5b88c36b75fef49e25c89 | mderose123 | |
2490fa8821291b5707bc1506cc6c3a215d4aa83b | mderose123 | |
33d6f50ceb97cb2b4d466be9060341070539936c | mderose123 | |
01fbb3543a9ea7c20d084f17bb6f5cd01547d0de | mderose123 | |
6c9124f6c8d6de2ae11845d280dd1fcab262ff3b | mderose123 | |
5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 | mderose123 | |
ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc | mderose123 | |
2c98d7b0624a97dc11085a5a8b703bb5338bb64c | mderose123 | |
23b923a6bc741e31720e10ea44cef4d00af30d8c | mderose123 | |
e61b0278b85a765d2749ad505de93fa90db3503c | mderose123 | |
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 … |
chipx86 | |
Please wrap your description and testing done sections to be 80 characters wide. |
david | |
I think, personally, this UI fits in really nicely with Review Board's pre-existing metaphors. Great job! A few notes: We … |
mike_conley | |
I think we should have the captions be last. We want to think of the UI in logical components. We … |
chipx86 | |
Col: 76 Missing semicolon. |
reviewbot | |
Col: 97 Missing semicolon. |
reviewbot | |
Col: 45 Missing semicolon. |
reviewbot | |
Col: 67 Missing semicolon. |
reviewbot | |
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 … |
chipx86 | |
Can you put this in alphabetical order? While not always appropriate for all CSS rules, it's a good point to … |
chipx86 | |
This is too generic a name, so it'll be especially important to namespace this, as per our docs. |
chipx86 | |
Blank line between these. |
chipx86 | |
Can you do testing on Firefox and Safari for these? |
chipx86 | |
Can you reorder these to be in alphabetical order? |
chipx86 | |
The blank line here should stay. |
chipx86 | |
Some important comments here: Keep line lengths in mind. Lines cannot go beyond 79 characters. These sorts of CSS changes … |
chipx86 | |
Blank line between these. |
chipx86 | |
Space after the if, before the (. |
chipx86 | |
Can you go through the file and make sure there's no extra whitespace (identified by these red blocks)? |
chipx86 | |
This would be best done as a _.template somewhere, using _.dedent (see other parts of the codebase for examples). |
chipx86 | |
Any text must be localized and provided in. This is easier done with templates. You can localize a string using: … |
chipx86 | |
Always use double quotes for attribute values, and never self-close tags. |
chipx86 | |
</> isn't valid. |
chipx86 | |
You can use chaining to reduce the code here: $('<div class="....">') .append($backgroundMenu) .append($resolutionMenu) .appendTo($header); |
chipx86 | |
You can set multiple model attributes at once: this.model.set({ checkeredBackground: false, background: color, }); This helps with ensuring that signal … |
chipx86 | |
This can be one statement. Also, code should always use single quotes instead of double quotes, unless the text in … |
chipx86 | |
Missing a period. This must also be one line (and can't go beyond the line length). |
chipx86 | |
This should probably be driven by the model. The model should listen for a change to checkeredBackground. If set, it … |
chipx86 | |
Col: 12 Unexpected ')'. |
reviewbot | |
Col: 12 Expected an identifier and instead saw ')'. |
reviewbot | |
Col: 13 Expected ')' and instead saw ';'. |
reviewbot | |
Col: 14 Missing semicolon. |
reviewbot | |
Col: 15 '$toolBar' is defined but never used. |
reviewbot | |
Let's use #rb-ns-ui.colors[@grey-40] instead of #808080 |
david | |
This should also use one of the #rb-ns-ui.colors definitions (see reviewboard/static/rb/css/ui/colors.less) instead of "grey" |
david | |
Please use single quotes instead of double. |
david | |
Please use single quotes instead of double. |
david | |
This should probably be called backgroundMenuTemplate. The $ prefix is for jQuery objects. |
david | |
We could skip defining this as a variable and just do: $('<div class="...">') .append(...) |
david | |
Let's wrap this like so: .append(backgroundMenuTemplate({ backgroundLabel: backgroundLabel, customLabel: customLabel, }) .append($resolutionMenu) ... Note that we also don't need quotes … |
david | |
Please add a trailing comma at the end of this. |
david | |
I only see one place each where we use these. We don't need to define new variables for them. Let's … |
chipx86 | |
A more standardized name (in our modern standard) would be -has-checkered-background. |
chipx86 | |
Do we need !important? |
chipx86 | |
Blank line between these. |
chipx86 | |
We usually make display the first, or one of the first, properties in a rule. Can you move it up? |
chipx86 | |
Where possible, let's use em units. These are going to scale better than pixel units, making them more accessible for … |
chipx86 | |
No blank line here. |
chipx86 | |
These are looking pretty long. Not uncommon for this to happen. Try: events: { ... 'click .rb-o-image-background-blah-blah-blah': '_onBlahChanged', ... } |
chipx86 | |
When you're setting a single attribute, pass each in as a parameter: this.model.set('checkeredBackground', false); |
chipx86 | |
this.$el is the .image-review-ui component, so rather than scanning the entire DOM for it each time, you can just use … |
chipx86 | |
This can now be: this.$el.toggleClass('-has-checkered-background', isCheckeredBackground); |
chipx86 | |
All this should be 1-space indentation. |
chipx86 | |
Only one space indentation. This should also be <%- backgroundLabel %>, in case the text gets localized to something that … |
chipx86 | |
The /> must be a >. |
chipx86 | |
for must align with class. |
chipx86 | |
This must also be <%-. |
chipx86 | |
This is indented 1 space too few. |
chipx86 | |
Rather than defining the variables above, you can pass in the strings here directly. |
chipx86 | |
Missing comma after this last line in the list. |
chipx86 | |
This could be one statement. |
chipx86 | |
This could be one statement. |
chipx86 | |
Doc summaries must be a single line. You can go into more detail in the description. I'd just say "changed … |
chipx86 | |
Col: 31 Missing semicolon. |
reviewbot |
- Groups:
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123
Checks run (2 succeeded)
- Description:
-
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. ~ Currently need to implement the action handler and fully integrate the user interface with the back end.
~ Currently need to debug the action handler(s) responsible for changing the background color.
- Description:
-
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. ~ Currently need to debug the action handler(s) responsible for changing the background color.
~ Currently need to debug the action handler(s) responsible for changing the background color. Please refer to implementation progress page for this project https://www.notion.so/reviewboard/Implementation-Progress-28120acec5c34674a271f973f94261b4
- 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 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123
- Description:
-
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. ~ Currently need to debug the action handler(s) responsible for changing the background color. Please refer to implementation progress page for this project https://www.notion.so/reviewboard/Implementation-Progress-28120acec5c34674a271f973f94261b4
~ Currently fixing two bugs: the image caption is no longer centered in the image header, and in a mobile phone view the image menus (zoom and background color) are not organized neatly. Please refer to implementation progress page for this project for more details. https://www.notion.so/reviewboard/Implementation-Progress-28120acec5c34674a271f973f94261b4
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 - Branch:
-
masterfeature---Background-Image-Colour-Selector
Checks run (2 succeeded)
-
-
I think, personally, this UI fits in really nicely with Review Board's pre-existing metaphors. Great job!
A few notes:
- We probably still want the filename in the center of this, rather than pushed to the right.
- The labels should be vertical-center aligned
- Either both should end with
:
or not. - 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.
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 - Diff:
-
Revision 8 (+443 -165)
- Removed Files:
- Added Files:
- Description:
-
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. ~ understand 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. - - Currently fixing two bugs: the image caption is no longer centered in the image header, and in a mobile phone view the image menus (zoom and background color) are not organized neatly. Please refer to implementation progress page for this project for more details. https://www.notion.so/reviewboard/Implementation-Progress-28120acec5c34674a271f973f94261b4
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123
Checks run (2 succeeded)
- 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.
~ 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).
-
-
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.
-
The
text-align
should predecede the nestedtable
rule, because it's relevant to.review-ui-header
.You'd also want a blank line between that and nested rules.
-
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.
-
-
-
-
-
-
Some important comments here:
- Keep line lengths in mind. Lines cannot go beyond 79 characters.
- These sorts of CSS changes are best done in CSS, not in JavaScript.
- Always use
const
orlet
instead ofvar
.
-
-
-
Can you go through the file and make sure there's no extra whitespace (identified by these red blocks)?
-
This would be best done as a
_.template
somewhere, using_.dedent
(see other parts of the codebase for examples). -
Any text must be localized and provided in. This is easier done with templates. You can localize a string using:
_`Background`
-
-
-
You can use chaining to reduce the code here:
$('<div class="....">') .append($backgroundMenu) .append($resolutionMenu) .appendTo($header);
-
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.
-
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.
-
-
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.
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123 2b2b0fd8f1ca6b8e2ee2f67e4eac0330415b0f81 mderose123
Checks run (2 succeeded)
- Summary:
-
[WIP] Background Color Selector When Viewing ImagesBackground Color Selector When Viewing Images
- 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 (i.e. single revision, "no diff", two-up, difference, split, and onion skin).
~ 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.
-
-
-
-
This should also use one of the
#rb-ns-ui.colors
definitions (seereviewboard/static/rb/css/ui/colors.less
) instead of "grey" -
-
-
-
-
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.
-
- Description:
-
~ 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. 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. ~ 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. - 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 (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.
~ 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. - Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123 2b2b0fd8f1ca6b8e2ee2f67e4eac0330415b0f81 mderose123 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123 2b2b0fd8f1ca6b8e2ee2f67e4eac0330415b0f81 mderose123 54ef3e77a645fda3f477f77643ecd38b0cbcddf2 mderose123 - Diff:
-
Revision 14 (+2200 -4598)
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 5221912914ed8de44527040095b1ea1005ec18d5 mderose123 c05bb2fc607176bda005d6f1b53aa453299d62e5 mderose123 3ee0b5af297052331e24868c48c1eb3cfa0b1c4e mderose123 e4138e2c62f6fb9e24757e02d11ba814081d5b9e mderose123 bf7a522dc5c790381e54497e632d60cfd1d6b7b6 mderose123 ffcd7b2bbcdf3028ffcc376b2386d658d917219f mderose123 71105512c62675d89fafb584902af4933f5eaad7 mderose123 2fd8f7a46ec54a04cad76d2a7b86475a403d2af5 mderose123 cd1cbd9eb7974b1f65b205459b93dd6df6e7e585 mderose123 310a9f5dd71dc2130a5ecd40937aa68c53778565 mderose123 928aae64a23a4682c40c732929465d68b7700c7f mderose123 151c26915a3973c305bd90063516972891e6b1c1 mderose123 cebdde16b851ae2d9b8be5358b9072f525f69bff mderose123 2b2b0fd8f1ca6b8e2ee2f67e4eac0330415b0f81 mderose123 54ef3e77a645fda3f477f77643ecd38b0cbcddf2 mderose123 f7188254230e300284e8085f30c177bda2664905 mderose123 a4f46881c26a25a75417cf57716c36e172bc010c mderose123 d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123 583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123 8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123 9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123 39fb276e93c847e4319821eb9d348cf549232563 mderose123 39a633b49bff00a1da86beac9691733449c19a14 mderose123 48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123 bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123 2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123 33d6f50ceb97cb2b4d466be9060341070539936c mderose123 01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123 6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123 5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123 ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123 2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123
Checks run (2 succeeded)
-
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?
-
-
I think we should have the captions be last.
We want to think of the UI in logical components. We have here, in order:
- Controls for manipulating the view
- Labels that describe the content below
- Controls for manipulating the view
- Content
It'd be much nicer as:
- Controls for manipulating the view
- Labels that describe the content below
- Content
-
-
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.
-
-
-
-
-
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.
-
-
These are looking pretty long. Not uncommon for this to happen.
Try:
events: { ... 'click .rb-o-image-background-blah-blah-blah': '_onBlahChanged', ... }
-
When you're setting a single attribute, pass each in as a parameter:
this.model.set('checkeredBackground', false);
-
this.$el
is the.image-review-ui
component, so rather than scanning the entire DOM for it each time, you can just usethis.$el
. -
-
-
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). -
-
-
-
-
-
-
-
- Commits:
-
Summary ID Author f7188254230e300284e8085f30c177bda2664905 mderose123 a4f46881c26a25a75417cf57716c36e172bc010c mderose123 d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123 583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123 8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123 9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123 39fb276e93c847e4319821eb9d348cf549232563 mderose123 39a633b49bff00a1da86beac9691733449c19a14 mderose123 48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123 bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123 2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123 33d6f50ceb97cb2b4d466be9060341070539936c mderose123 01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123 6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123 5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123 ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123 2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123 f7188254230e300284e8085f30c177bda2664905 mderose123 a4f46881c26a25a75417cf57716c36e172bc010c mderose123 d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123 583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123 8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123 9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123 39fb276e93c847e4319821eb9d348cf549232563 mderose123 39a633b49bff00a1da86beac9691733449c19a14 mderose123 48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123 bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123 2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123 33d6f50ceb97cb2b4d466be9060341070539936c mderose123 01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123 6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123 5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123 ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123 2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123 23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123
- Commits:
-
Summary ID Author f7188254230e300284e8085f30c177bda2664905 mderose123 a4f46881c26a25a75417cf57716c36e172bc010c mderose123 d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123 583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123 8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123 9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123 39fb276e93c847e4319821eb9d348cf549232563 mderose123 39a633b49bff00a1da86beac9691733449c19a14 mderose123 48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123 bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123 2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123 33d6f50ceb97cb2b4d466be9060341070539936c mderose123 01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123 6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123 5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123 ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123 2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123 23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123 f7188254230e300284e8085f30c177bda2664905 mderose123 a4f46881c26a25a75417cf57716c36e172bc010c mderose123 d1d129a1d3f5b4c780331a29f890cb1dc22728ee mderose123 583727ca00da0095be8b3b244a08c68a9ecf2921 mderose123 8c99f19399f2b0b7dfb94bb4c959c8db5c72523c mderose123 9a0dd0e977d97ad7702ea5cc5eb517d803576304 mderose123 39fb276e93c847e4319821eb9d348cf549232563 mderose123 39a633b49bff00a1da86beac9691733449c19a14 mderose123 48fb7fe485a5fc74c7a3f8630b830fafaa8651ee mderose123 bee7caae3cecf3f88df5b88c36b75fef49e25c89 mderose123 2490fa8821291b5707bc1506cc6c3a215d4aa83b mderose123 33d6f50ceb97cb2b4d466be9060341070539936c mderose123 01fbb3543a9ea7c20d084f17bb6f5cd01547d0de mderose123 6c9124f6c8d6de2ae11845d280dd1fcab262ff3b mderose123 5ccdcdcba6ba5f627d88effdc1a8bdc71ecfe6d1 mderose123 ad5b3a07a8fedc817bfddbdd502c1cdcdeb0d2fc mderose123 2c98d7b0624a97dc11085a5a8b703bb5338bb64c mderose123 23b923a6bc741e31720e10ea44cef4d00af30d8c mderose123 e61b0278b85a765d2749ad505de93fa90db3503c mderose123 4f1dd30545cf9ae229884ec81b7efea762c465c4 mderose123