#63 - Add Visual affordance for required photo upload on /setup page #67
Conversation
|
@samholmes when you are free, this PR will be ready for you and hope this will be good by Saturday's monthly meetup 🙂 |
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
These requested changes are minor but worthwhile. Great work!
|
|
||
| const FieldError = styled.p` | ||
| color: #f87171; | ||
| font-size: 0.75rem; |
There was a problem hiding this comment.
font-size here is not consistent with font-size in TextInput.
| ` | ||
|
|
||
| const FieldError = styled.p` | ||
| color: #f87171; |
There was a problem hiding this comment.
Consider defining a color --error-color: #f87171 in :root declaration in globals.css. This way we have a consistent color as part of our theming.
|
|
||
| const FieldError = styled.p` | ||
| color: #f87171; | ||
| font-size: 0.875rem; |
There was a problem hiding this comment.
font-size 0.75rem to be consistent with the other FieldError in the nametag?
| onImageUpload={async (file) => { | ||
| const url = await handleImageUpload(file) | ||
| // Clear photo error immediately when photo is uploaded | ||
| if (hasAttemptedSubmit && validationErrors.profilePhoto) { | ||
| setValidationErrors((prev) => ({ ...prev, profilePhoto: false })) | ||
| } | ||
| return url | ||
| }} |
There was a problem hiding this comment.
Instead of inlining a new handler function, move your implementation to the handleImageUpload handler. Rework your changes into that existing handler.
| onDataChange={(data) => { | ||
| setNametagData(data) | ||
| // Clear errors when user provides valid data | ||
| if (hasAttemptedSubmit) { | ||
| setValidationErrors((prev) => ({ | ||
| ...prev, | ||
| profilePhoto: !data.profilePhoto, | ||
| fullName: !data.fullName.trim() | ||
| })) | ||
| } | ||
| }} |
There was a problem hiding this comment.
Instead of inlining a new handler function, move your implementation to the a handleDataChange handler.
| onChange={(e) => { | ||
| setHandle(e.target.value.toLowerCase()) | ||
| // Clear error when user starts typing | ||
| if (hasAttemptedSubmit && validationErrors.handle) { | ||
| setValidationErrors((prev) => ({ ...prev, handle: false })) | ||
| } | ||
| }} |
There was a problem hiding this comment.
Move this to a handleHandleChange. Hah, that's a funny name. Typical naming convention for handler functions is handle[semantic][event], in this case the semantic is "handle" because that's what we call the username, and the event is "Change" because it's for "onChange". Hence the weird sounding name.
| if (props.$error) { | ||
| return "#f87171" | ||
| } | ||
| return props.$variant === "secondary" ? "white" : "rgba(0, 0, 0, 0.4)" |
There was a problem hiding this comment.
Might as well change the ternary here to an if statement while we're at it.
| if (props.$error) { | ||
| return "1px solid #f87171" | ||
| } | ||
| return props.$variant === "secondary" |
There was a problem hiding this comment.
Consider replacing ternary to if statement
| <NametagLeft> | ||
| <PhotoFrame onClick={readOnly ? undefined : () => fileInputRef.current?.click()}> | ||
| <PhotoFrame | ||
| onClick={readOnly ? undefined : () => fileInputRef.current?.click()} |
There was a problem hiding this comment.
Define a handlePhotoFrameClick handler function and implement the logic there.
2 participants
Created red borders for required fields (See below screenshot):
Ensured that following original behaviors are not impacted: