-
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-675 Make widget component "reusable" #2579
Conversation
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.
Solid work! I have a few minor things I'd like you to revise. but once the test errors are resolved I'd say this is good to go.
@@ -29,6 +29,7 @@ class Widget(BaseModel): # pylint: disable=too-few-public-methods | |||
title = db.Column(db.String(100), comment='Custom title for the widget.') | |||
items = db.relationship('WidgetItem', backref='widget', cascade='all, delete', order_by='WidgetItem.sort_index') | |||
sort_index = db.Column(db.Integer, nullable=False, default=1) |
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.
Does it still make sense to have a sort_index on a widget if its position is determined by its location?
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.
Solid question - I kept it so we could:
- use the old widget picker
- allow for sorting in the future, which the PO says is a possibility
That said, once we know the fate of the old picker and widgets going forward, a clean-up ticket would be warranted
import { WidgetType } from 'models/widget'; | ||
import { DOCUMENT_TYPE } from 'models/document'; | ||
import { WidgetLocation } from 'models/widget'; |
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.
import { WidgetType } from 'models/widget'; | |
import { DOCUMENT_TYPE } from 'models/document'; | |
import { WidgetLocation } from 'models/widget'; | |
import { WidgetType, WidgetLocation } from 'models/widget'; | |
import { DOCUMENT_TYPE } from 'models/document'; |
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 catching this!
@@ -12,11 +12,12 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; | |||
import { faUserGroupSimple } from '@fortawesome/pro-regular-svg-icons/faUserGroupSimple'; | |||
import { useCreateWidgetMutation } from 'apiManager/apiSlices/widgets'; | |||
import { optionCardStyle } from '../constants'; | |||
import { WidgetLocation } from 'models/widget'; |
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.
Merge this import with line 9
dispatch( | ||
openNotificationModal({ | ||
open: true, | ||
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.
data: { | |
data: { | |
style: 'warning', |
@@ -178,6 +180,10 @@ export const ConfigSummary = () => { | |||
</Grid> | |||
</OutlineBox> | |||
</Grid> | |||
<Grid item> | |||
<WidgetPicker location={WidgetLocation.engagementAuthoring} /> |
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.
I assume in the future these location variables will be based on where the widget will render on the public view, rather than where it's edited on the backend
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.
Whichever makes the most sense to us, the devs. But probably, yes! Whoever starts adding the widget picker to the editor can make the call as to the naming
Quality Gate passedIssues Measures |
API tests have been failing for a while. I created a ticket to look into it: https://citz-gdx.atlassian.net/browse/DESENG-699 Will be going ahead with merging |
* DESENG-675 Make widget component "reusable" * DESENG-675 Update changelog, contributing, met-web tests * DESENG-675 Merge import statements, add warning style to widget deletion
Issue #: https://citz-gdx.atlassian.net/browse/DESENG-675
Description of changes:
Tips for reviewers
met-web/src/components/engagement/admin/create/widgets/*
and the rest are just updating dependencies and adding a few props/flags to support the new functionalityUser Guide update ticket (if applicable): N/A