-
Notifications
You must be signed in to change notification settings - Fork 19
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
DESENG-689: Add image widget #2586
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2586 +/- ##
==========================================
+ Coverage 75.74% 75.80% +0.05%
==========================================
Files 603 607 +4
Lines 21811 21881 +70
Branches 1811 1810 -1
==========================================
+ Hits 16521 16587 +66
- Misses 5028 5032 +4
Partials 262 262
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Should we create a ticket for documentation in the user guide? Anything holding us back from that? |
Created a ticket - https://citz-gdx.atlassian.net/browse/DESENG-705 |
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.
Thanks for this Nat! Also nice of you to keep adding support for the old engagement view. We may want to try to save ourselves some work and ask Steve if we need to keep doing that.
Before proceeding, could you please just let me know:
- If all form labels are properly associated with controls
- If the uploaded image gets deleted when a widget is deleted. We'll need to make sure we're doing that
setValue('image_url', '', { shouldValidate: true, shouldDirty: true, shouldTouch: true }); | ||
}; | ||
|
||
const handleUploadImageFile = async () => { |
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.
When a widget is deleted, does the corresponding uploaded image get deleted?
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.
No; there is currently no mechanism for this in MET
met-web/src/components/engagement/form/EngagementWidgets/Image/Form.tsx
Outdated
Show resolved
Hide resolved
<form onSubmit={uploadImageAndSubmit}> | ||
<Grid container direction="row" alignItems={'flex-start'} justifyContent="flex-start" spacing={2}> | ||
<Grid item xs={12}> | ||
<MetLabel>Description</MetLabel> |
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.
Form label elements require some mechanism for associating them with inputs. Please review: https://www.w3.org/WAI/tutorials/forms/labels/
I know TextField may have a mechanism for autogenerating its own label, I'm not sure if that's at play here.
name="image_url" | ||
control={control} | ||
render={({ field }) => ( | ||
<ImageUpload |
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.
Jareth seemed to say that there wasn't an easy way to label the ImageUpload component. If that's the case then don't worry too much about labelling it if it's work. I've created a separate ticket for it
} | ||
const response = await http.PostRequest<GeoJSON>(Endpoints.Maps.SHAPEFILE_PREVIEW, formdata); | ||
const response = await http.PostRequest<GeoJSON>(url, formdata, {}, { 'Content-type': 'multipart/form-data' }); |
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.
Was the linter throwing errors here?
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.
This was a small bugfix I implemented to fix an issue with file uploading on the Map widget that turned out not to be related to the Image widget the way I thought it was. I accidentally left it in this PR; should I remove it?
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.
You don't need to review it but for small, unrelated fixes, the PR description is a great place to mention those!
✅ All form controls now have labels associated where possible.
☑️ Ticket created to follow up with deleting images from widgets, engagements, etc |
…/Form.tsx Co-authored-by: Baelx <alexander.wintschel@gov.bc.ca>
Quality Gate passedIssues Measures |
Issue #: 🎟️ DESENG-689
Description of changes:
User Guide update ticket (if applicable):
https://citz-gdx.atlassian.net/browse/DESENG-705