LG-11984: document capture selfie hint text#9901
Conversation
amirbey
changed the title
charleyf
left a comment
charleyf
left a comment
There was a problem hiding this comment.
This looks good to me. I did help write this though, so it would be nice (not required IMO) for someone else to review.
app/javascript/packages/document-capture/components/acuant-capture.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I keep waffling whether it's better to have this function, or remove it and just use setImageCaptureText 🤷
There was a problem hiding this comment.
I think it's six one way and a half a dozen the other. 🤷🏻♀️
There was a problem hiding this comment.
Good catch, these strings should not be embedded directly.
There was a problem hiding this comment.
| return isReady ? ( | |
| // The <p> to display the text over the selfie capture can be moved to another component | |
| // without too much work, putting it here to keep it conceptually grouped with the selfie capture canvas | |
| return isReady ? ( |
There was a problem hiding this comment.
Either way, personally think it's too much for an additional component just for 3 lines of static code, actually just one line.
There was a problem hiding this comment.
Hm, interesting. Seems like that comment might be more confusing than it's worth. Agreed that the <p> should't be it's own component.
What I meant was that it actually doesn't matter where the <p> is (as long as it's in the selfie components) because the CSS removes it from the document flow.
config/locales/doc_auth/es.yml
Outdated
There was a problem hiding this comment.
Is the single quote needed?
There was a problem hiding this comment.
I think it's six one way and a half a dozen the other. 🤷🏻♀️
amirbey
force-pushed
the
amirbey/LG-11984-selfi-hint-text
branch
from
6114684 to
db3f7ef
Compare
changelog: Upcoming Features, Document Authentication, Show user hints during selfie capture
Co-authored-by: Charles Ferguson <charles.ferguson@gsa.gov>
…ture.scss Co-authored-by: Charley Ferguson <charleyferguson@navapbc.com>
amirbey
force-pushed
the
amirbey/LG-11984-selfi-hint-text
branch
from
db3f7ef to
aeef738
Compare
| return isReady ? ( | ||
| <div id={acuantCaptureContainerId} /> | ||
| <div id={acuantCaptureContainerId}> | ||
| <p className="document-capture-selfie-feedback">{imageCaptureText}</p> |
There was a problem hiding this comment.
Our use of BEM naming conventions aligns neatly 1-to-1 with React components, where if we had a CSS class name of document-capture-selfie-feedback, I'd expect it to be assigned in a component DocumentCaptureSelfieFeedback. I think that could make sense to split out as a component. As currently implemented, I'd assume it would be called something like acuant-selfie-capture-canvas__selfie-feedback, being an element (BEM) within AcuantSelfieCaptureCanvas.
| @extend %pad-common-id-card; | ||
| } | ||
| // Styles for the text that appears over the selfie capture screen to help users position their face for a good photo | ||
| .document-capture-selfie-feedback { |
There was a problem hiding this comment.
Following prior comment, an advantage of BEM is it generally avoids issues with needing to qualify (nest) CSS selectors, so I'd expect this to be defined at a top-level.
5 participants
🎫 Ticket
LG-11984
🛠 Summary of changes
Display the selfie capture hint text to the user on the acuant capture canvas.
📜 Testing Plan
👀 Screenshots