Fix #2129, notify user when full page is cut off #3550
Conversation
ianb
force-pushed
the
image-crop-warning
branch
from
a17dea2
to
1e7431b
Compare
|
Or with silly scales:
|
ianb
force-pushed
the
image-crop-warning
branch
from
1e7431b
to
d545e9e
Compare
|
new font size:
|
static/css/inline-selection.scss
Outdated
| @@ -402,6 +402,20 @@ $very-light-grey: #ededed; | |||
| width: 400px; | |||
| } | |||
|
|
|||
| #imageCroppedWarning { | |||
| background: rgba(0,0,0,.8); | |||
There was a problem hiding this comment.
Could you specify the opacity separately? Seems to make Windows High Contrast mode behave in a more reasonable way
There was a problem hiding this comment.
I don't think you can make the background opacity separate, only the entire element opacity...?
shared/shot.js
Outdated
| @@ -707,7 +707,7 @@ class _Clip { | |||
| } | |||
| assert(checkObject(image, ["url"], ["dimensions", "text", "location", "captureType", "type"]), "Bad attrs for Clip Image:", Object.keys(image)); | |||
| assert(isValidClipImageUrl(image.url), "Bad Clip image URL:", image.url); | |||
| assert(image.captureType == "madeSelection" || image.captureType == "selection" || image.captureType == "visible" || image.captureType == "auto" || image.captureType == "fullPage" || !image.captureType, "Bad image.captureType:", image.captureType); | |||
| assert(image.captureType == "madeSelection" || image.captureType == "selection" || image.captureType == "visible" || image.captureType == "auto" || image.captureType == "fullPage" || image.captureType == "fullPageTruncated" || !image.captureType, "Bad image.captureType:", image.captureType); | |||
There was a problem hiding this comment.
this line is looking awfully long
| @@ -16,6 +16,8 @@ notificationLinkCopiedTitle = Link Copied | |||
| # modifier key used in the shortcut (do not translate it): for example, Ctrl-V | |||
| # on Windows systems. | |||
| notificationLinkCopiedDetails = The link to your shot has been copied to the clipboard. Press {meta_key}-V to paste. | |||
| # This is displayed as a warning when a Full Page save will be cropped | |||
There was a problem hiding this comment.
Could also mention that {pixels} is a placeholder for a plural whole number, like 5000.
| @@ -160,22 +160,28 @@ this.uicontrol = (function() { | |||
| }, | |||
| onClickFullPage: () => { | |||
| sendEvent("capture-full-page", "selection-button"); | |||
| captureType = 'fullPage'; | |||
There was a problem hiding this comment.
y u no double quotes in this file?
static/css/inline-selection.scss
Outdated
| text-align: center; | ||
| width: 100%; | ||
| z-index: 2; | ||
| position: absolute; |
| @@ -16,6 +16,8 @@ notificationLinkCopiedTitle = Link Copied | |||
| # modifier key used in the shortcut (do not translate it): for example, Ctrl-V | |||
| # on Windows systems. | |||
| notificationLinkCopiedDetails = The link to your shot has been copied to the clipboard. Press {meta_key}-V to paste. | |||
| # This is displayed as a warning when a Full Page save will be cropped | |||
| imageCroppedWarning = This image has been cropped to {pixels} pixels. | |||
There was a problem hiding this comment.
Should the string be, "This image will be cropped to {pixels} pixels" ?
There was a problem hiding this comment.
If you're using "pixel", you should use a proper plural form. The easy way out is to use px.
http://projectfluent.io/fluent/guide/selectors.html
Also CC @stasm to make sure we don't break anything if you go for proper plural.
There was a problem hiding this comment.
@flodolo In this case the singular will never be needed, so I'm not sure I see the need for the extra selector syntax
There was a problem hiding this comment.
@johngruen said "px", but I thought that encouraged a term that a normal human wouldn't know so I switched to pixels.
There was a problem hiding this comment.
Once again, stop using your English brain to think ;-)
one vs many is not a plural form, it's something that works in a bunch of languages
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms
|
This is for uplift to 57, so I think in a meeting we decided not to mess with anything unnecessarily (like the pixel limit) |
Adds a new captureType, fullPageTruncated
ianb
force-pushed
the
image-crop-warning
branch
from
d545e9e
to
aa97577
Compare
This sounds like this change is supposed to be in Firefox 57 but I cannot see the message. Before filing a bug because of truncated full page screenshots I found this change. I cannot see the message in Firefox 57 on Ubuntu. Should I file a bug or will this be available in a future release? |
|
@justfortherec It will be in a future release. |
Adds a new captureType, fullPageTruncated
Looks like this: