Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
 * Set required font-family/size via uswds class
 * Use uswds margin classes instead of `smart-hub--*` classes
 * Wrap group of `await` calls in `Promise.all` inside resource
migration
 * Remove File `attachmentType` field
 * Change trashcan icon's color to black to match other trashcan icons
  • Loading branch information
jasalisbury committed Mar 5, 2021
1 parent 9438d3f commit 64db2a9
Show file tree
Hide file tree
Showing 21 changed files with 71 additions and 127 deletions.
3 changes: 0 additions & 3 deletions docs/openapi/index.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,6 @@ components:
fileUpload:
type: object
properties:
attachmentType:
type: string
description: "Type of attachment. Acceptable values are ATTACHMENT or RESOURCE"
reportId:
type: number
description: "id of the Activity report the file is associated with"
Expand Down
11 changes: 2 additions & 9 deletions frontend/src/components/FileUploader.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ import { uploadFile, deleteFile } from '../fetchers/File';

import './FileUploader.css';

export const upload = async (file, reportId, attachmentType, setErrorMessage) => {
export const upload = async (file, reportId, setErrorMessage) => {
let res;
try {
const data = new FormData();
data.append('reportId', reportId);
data.append('attachmentType', attachmentType);
data.append('file', file);
res = await uploadFile(data);
} catch (error) {
Expand All @@ -42,13 +41,7 @@ export const handleDrop = async (e, reportId, id, onChange, setErrorMessage) =>
setErrorMessage('Cannot save attachments without a Grantee or Non-Grantee selected');
return;
}
let attachmentType;
if (id === 'attachments') {
attachmentType = 'ATTACHMENT';
} else if (id === 'otherResources') {
attachmentType = 'RESOURCE';
}
const newFiles = e.map((file) => upload(file, reportId, attachmentType, setErrorMessage));
const newFiles = e.map((file) => upload(file, reportId, setErrorMessage));
Promise.all(newFiles).then((values) => {
onChange(values);
});
Expand Down
2 changes: 0 additions & 2 deletions frontend/src/components/FormItem.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
.smart-hub--form-required {
font-family: SourceSansPro;
font-size: 16px;
color: #d42240;
}
2 changes: 1 addition & 1 deletion frontend/src/components/FormItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function FormItem({
const labelWithRequiredTag = (
<>
{label}
{required && (<span className="smart-hub--form-required"> (Required)</span>)}
{required && (<span className="smart-hub--form-required font-family-sans font-ui-xs"> (Required)</span>)}
</>
);

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/__tests__/FileUploader.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('upload tests', () => {
const mockSetErrorMessage = jest.fn();
it('can upload a file and return the correct information', async () => {
const mockFileUpload = jest.spyOn(fileFetcher, 'uploadFile').mockImplementation(async () => ({ id: 1, url: 'url' }));
const got = await upload(mockFile, 1, 'fakeAttachment', mockSetErrorMessage);
const got = await upload(mockFile, 1, mockSetErrorMessage);
expect(got).toStrictEqual({
id: 1, originalFileName: mockFile.name, fileSize: mockFile.size, status: 'UPLOADED', url: 'url',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const Review = ({
</p>
</div>
<Form className="smart-hub--form-large" onSubmit={handleSubmit(onFormReview)}>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Review and submit report">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Review and submit report">
<Label htmlFor="managerNotes">Manager notes</Label>
<Textarea inputRef={register} id="managerNotes" name="managerNotes" className={textAreaClass} />
</Fieldset>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const Draft = ({
<>
<h2>Submit Report</h2>
<Form className="smart-hub--form-large" onSubmit={handleSubmit(onSubmit)}>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Additional Notes">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Additional Notes">
<FormItem
label="Creator notes"
name="additionalNotes"
Expand All @@ -46,7 +46,7 @@ const Draft = ({
<Textarea inputRef={register} id="additionalNotes" name="additionalNotes" className={textAreaClass} />
</FormItem>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Review and submit report">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Review and submit report">
<p className="margin-top-4">
Submitting this form for approval means that you will no longer be in draft
mode. Please review all information in each section before submitting to your
Expand Down
32 changes: 16 additions & 16 deletions frontend/src/pages/ActivityReport/Pages/activitySummary.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ const ActivitySummary = ({
<Helmet>
<title>Activity summary</title>
</Helmet>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Who was the activity for?">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Who was the activity for?">
<div id="activity-for" />
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Was this activity for a grantee or non-grantee?"
name="activityRecipientType"
Expand All @@ -114,7 +114,7 @@ const ActivitySummary = ({
/>
</FormItem>
</div>
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label={recipientLabel}
name="activityRecipients"
Expand All @@ -131,7 +131,7 @@ const ActivitySummary = ({
/>
</FormItem>
</div>
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Collaborating Specialists"
name="collaborators"
Expand All @@ -150,7 +150,7 @@ const ActivitySummary = ({
</div>
{granteeSelected
&& (
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Program type(s)"
name="programTypes"
Expand All @@ -165,7 +165,7 @@ const ActivitySummary = ({
</FormItem>
</div>
)}
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Target Populations addressed. You may choose more than one."
name="targetPopulations"
Expand All @@ -180,9 +180,9 @@ const ActivitySummary = ({
</FormItem>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Reason for Activity">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Reason for Activity">
<div id="reasons" />
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Who requested this activity? Use &quot;Regional Office&quot; for TTA not requested by grantee."
name="requester"
Expand All @@ -206,7 +206,7 @@ const ActivitySummary = ({
/>
</FormItem>
</div>
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="Reason(s). You may choose more than one."
name="reason"
Expand All @@ -219,7 +219,7 @@ const ActivitySummary = ({
</FormItem>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Activity date">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Activity date">
<div id="date" />
<div>
<Grid row gap>
Expand Down Expand Up @@ -274,9 +274,9 @@ const ActivitySummary = ({
</Grid>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Training or Technical Assistance">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Training or Technical Assistance">
<div id="tta" />
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="What TTA was provided"
name="ttaType"
Expand All @@ -286,7 +286,7 @@ const ActivitySummary = ({
{renderCheckbox('ttaType', 'technical-assistance', 'Technical Assistance', 'Please specify the type of TTA provided')}
</FormItem>
</div>
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label="How was the activity conducted?"
name="deliveryMethod"
Expand All @@ -311,7 +311,7 @@ const ActivitySummary = ({
</FormItem>
<div aria-live="polite">
{isVirtual && (
<div className="smart-hub--form-group smart-hub--virtual-delivery-group">
<div className="margin-top-2 smart-hub--virtual-delivery-group">
<FormItem
label="Please specify how the virtual event was conducted."
name="virtualDeliveryType"
Expand Down Expand Up @@ -339,9 +339,9 @@ const ActivitySummary = ({
</div>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Participants">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Participants">
<div id="other-participants" />
<div className="smart-hub--form-group">
<div className="margin-top-2">
<FormItem
label={participantsLabel}
name="participants"
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/pages/ActivityReport/Pages/components/Goal.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const Goals = ({ id, name, onRemove }) => (
</div>
<div className="margin-left-auto margin-top-2">
<Button onClick={(e) => { e.preventDefault(); onRemove(id); }} unstyled className="smart-hub--button" aria-label="remove goal">
<FontAwesomeIcon color="gray" icon={faTrash} />
<FontAwesomeIcon color="black" icon={faTrash} />
</Button>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const GoalPicker = ({
const uniqueAvailableGoals = uniqBy(allAvailableGoals, 'id');

return (
<div className="smart-hub--form-section">
<div className="margin-top-4">
<FormItem
label="You must select an established goal(s) OR create a new goal for this activity."
name="goals"
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/pages/ActivityReport/Pages/goalsObjectives.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ const GoalsObjectives = ({
</Helmet>
{activityRecipientType === 'grantee' && hasGrants
&& (
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Goals and objectives">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Goals and objectives">
<div id="goals-and-objectives" />
<GoalPicker
availableGoals={availableGoals}
/>
</Fieldset>
)}
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Context">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Context">
<Label htmlFor="context">OPTIONAL: Provide background or context for this activity</Label>
<Textarea id="context" name="context" inputRef={register()} />
</Fieldset>
Expand Down
14 changes: 7 additions & 7 deletions frontend/src/pages/ActivityReport/Pages/topicsResources.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ const TopicsResources = ({
<Helmet>
<title>Topics and resources</title>
</Helmet>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Topics Covered">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Topics Covered">
<div id="topics-covered" />
<div className="smart-hub--form-section">
<div className="margin-top-4">
<FormItem
label="Topic(s) covered. You may choose more than one."
name="topics"
Expand All @@ -46,9 +46,9 @@ const TopicsResources = ({
</FormItem>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="OHS / ECLKC resources">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="OHS / ECLKC resources">
<div id="ECLKCResources" />
<div className="smart-hub--form-section">
<div className="margin-top-4">
<Label>
Resources from OHS / ECLKC
<br />
Expand All @@ -62,9 +62,9 @@ const TopicsResources = ({
</Label>
</div>
</Fieldset>
<Fieldset className="smart-hub--report-legend smart-hub--form-section" legend="Non-ECLKC resources">
<Fieldset className="smart-hub--report-legend margin-top-4" legend="Non-ECLKC resources">
<div id="nonECLKCResources" />
<div className="smart-hub--form-section">
<div className="margin-top-4">
<Label>
For non-ECLKC resources enter URL.
<br />
Expand All @@ -76,7 +76,7 @@ const TopicsResources = ({
</Label>
</div>
</Fieldset>
<Fieldset legend="Supporting attachments" className="smart-hub--report-legend smart-hub--form-section">
<Fieldset legend="Supporting attachments" className="smart-hub--report-legend margin-top-4">
<div id="attachments" />
<Label htmlFor="attachments">Upload resources not available online, agenda, service plans, sign-in sheets, etc.</Label>
<Controller
Expand Down
7 changes: 0 additions & 7 deletions frontend/src/pages/ActivityReport/index.css
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
.smart-hub--form-group {
margin-top: 1.5rem;
}

.smart-hub--report-checkbox {
margin-top: 1.25rem;
}

.smart-hub--form-section {
margin-top: 2rem;
}

.smart-hub--remove-resource {
margin-top: 8px !important;
margin-left: 4px;
Expand Down
32 changes: 18 additions & 14 deletions src/migrations/20210302170502-add-resources-to-activity-report.js
Original file line number Diff line number Diff line change
@@ -1,25 +1,29 @@
module.exports = {
up: async (queryInterface, Sequelize) => {
queryInterface.sequelize.transaction(async (transaction) => {
await queryInterface.addColumn('ActivityReports',
'nonECLKCResourcesUsed',
{
type: Sequelize.ARRAY(Sequelize.STRING),
}, { transaction });
await queryInterface.removeColumn('ActivityReports', 'resourcesUsed', { transaction });
await queryInterface.addColumn('ActivityReports',
'ECLKCResourcesUsed',
{
type: Sequelize.ARRAY(Sequelize.STRING),
}, { transaction });
await Promise.all([
queryInterface.addColumn('ActivityReports',
'nonECLKCResourcesUsed',
{
type: Sequelize.ARRAY(Sequelize.STRING),
}, { transaction }),
queryInterface.removeColumn('ActivityReports', 'resourcesUsed', { transaction }),
queryInterface.addColumn('ActivityReports',
'ECLKCResourcesUsed',
{
type: Sequelize.ARRAY(Sequelize.STRING),
}, { transaction }),
]);
});
},

down: async (queryInterface, Sequelize) => {
queryInterface.sequelize.transaction(async (transaction) => {
await queryInterface.removeColumn('ActivityReports', 'nonECLKCResourcesUsed', { transaction });
await queryInterface.removeColumn('ActivityReports', 'ECLKCResourcesUsed', { transaction });
await queryInterface.addColumn('ActivityReports', 'resourcesUsed', { type: Sequelize.TEXT }, { transaction });
await Promise.all([
queryInterface.removeColumn('ActivityReports', 'nonECLKCResourcesUsed', { transaction }),
queryInterface.removeColumn('ActivityReports', 'ECLKCResourcesUsed', { transaction }),
queryInterface.addColumn('ActivityReports', 'resourcesUsed', { type: Sequelize.TEXT }, { transaction }),
]);
});
},
};
18 changes: 18 additions & 0 deletions src/migrations/20210305174257-remove-attachment-type.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module.exports = {
up: async (queryInterface) => {
queryInterface.sequelize.transaction((t) => Promise.all([
queryInterface.removeColumn('Files', 'attachmentType', { transaction: t }),
queryInterface.sequelize.query('DROP TYPE public."enum_Files_attachmentType";', { transaction: t }),
]));
},

down: async (queryInterface, Sequelize) => {
queryInterface.addColumn(
'Files',
'attachmentType',
{
type: Sequelize.DataTypes.ENUM(['ATTACHMENT', 'RESOURCE']),
},
);
},
};
1 change: 0 additions & 1 deletion src/models/activityReport.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export default (sequelize, DataTypes) => {
});
ActivityReport.belongsTo(models.Region, { foreignKey: 'regionId', as: 'region' });
ActivityReport.hasMany(models.File, { foreignKey: 'activityReportId', as: 'attachments' });
ActivityReport.hasMany(models.File, { foreignKey: 'activityReportId', as: 'otherResources' });
ActivityReport.hasMany(models.NextStep, { foreignKey: 'activityReportId', as: 'specialistNextSteps' });
ActivityReport.hasMany(models.NextStep, { foreignKey: 'activityReportId', as: 'granteeNextSteps' });
ActivityReport.belongsToMany(models.Goal, {
Expand Down
4 changes: 0 additions & 4 deletions src/models/file.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,6 @@ module.exports = (sequelize, DataTypes) => {
),
allowNull: false,
},
attachmentType: {
type: DataTypes.ENUM('ATTACHMENT', 'RESOURCE'),
allowNull: false,
},
// File size in bytes
fileSize: {
type: DataTypes.INTEGER,
Expand Down

0 comments on commit 64db2a9

Please sign in to comment.