-
Notifications
You must be signed in to change notification settings - Fork 167
LG-10362: New Image Capture Page Language #8851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
charleyf
merged 8 commits into
main
from
charley/lg-10362/new-image-capture-target-language
Jul 27, 2023
+16
−16
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8d89b7b
Update english text
charleyf 476d619
Update spanish text
charleyf b37c129
Update french text
charleyf 712dbc3
Allow lowercase text in capture box
charleyf f7c4b21
Lint yaml files
charleyf bb5fc07
changelog: User-Facing Improvements, In-Person Proofing, Update text …
charleyf 721c696
Use the font-size from %h2, which is different on mobile and desktop.
charleyf 624b234
Merge branch 'main' into charley/lg-10362/new-image-capture-target-la…
charleyf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kellular Putting the screenshots here so we can use the threading to discuss. I did also want to mention that this PR changes from all uppercase like "FRONT" to mixed case as I'm seeing in the designs.
iOS - Chrome
iOS - Safari
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Yep, the tap target copy should be sentence case like you have it here.
Is the desktop screen size using an h1 for the tap target copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the desktop in Chrome
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy to resize this text, happy to give you a live demo of various sizes. Here's some screenshots with different font sizes I tried.
Tap-Target Text 1.25rem

Tap-Target Text 1.5rem

Tap-Target Text As Is 1.625rem

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for all the screenshots! Could we make the tap target text an h2 for both desktop and mobile? I think that's 1.375rem and 1.25rem respectively
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made that change (commit link). The size of that text is now directly inherited from the
h2sizing. Here's what it looks likeDesktop

Mobile

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! And just confirming - the tap target on desktop screen sizes will still include the "Drag file here or choose from folder" text right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! That happened because I was using dev-tools on my desktop to view this and forgot to refresh the page. Here's what it looks like on my desktop right now

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thanks for confirming! This LGTM