Skip to content
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

Merged
merged 3 commits into from
Aug 22, 2024
Merged

DESENG-675 Make widget component "reusable" #2579

merged 3 commits into from
Aug 22, 2024

Conversation

Baelx
Copy link
Collaborator

@Baelx Baelx commented Aug 22, 2024

Issue #: https://citz-gdx.atlassian.net/browse/DESENG-675

Description of changes:

  • Widget component can now be placed anywhere within an engagement
  • Update Changelog to include instructions for use

Tips for reviewers

  • The PR is smaller than it looks, most substantive changes are under 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 functionality

User Guide update ticket (if applicable): N/A

Copy link
Contributor

@NatSquared NatSquared left a 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)
Copy link
Contributor

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?

Copy link
Collaborator Author

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

Comment on lines 10 to 12
import { WidgetType } from 'models/widget';
import { DOCUMENT_TYPE } from 'models/document';
import { WidgetLocation } from 'models/widget';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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';

Copy link
Collaborator Author

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';
Copy link
Contributor

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: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data: {
data: {
style: 'warning',

@@ -178,6 +180,10 @@ export const ConfigSummary = () => {
</Grid>
</OutlineBox>
</Grid>
<Grid item>
<WidgetPicker location={WidgetLocation.engagementAuthoring} />
Copy link
Contributor

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

Copy link
Collaborator Author

@Baelx Baelx Aug 22, 2024

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 73.07692% with 7 lines in your changes missing coverage. Please review.

Project coverage is 76.03%. Comparing base (e2d09dd) to head (0d86c29).

Files Patch % Lines
...form/EngagementWidgets/Documents/AddFileDrawer.tsx 50.00% 1 Missing ⚠️
...m/EngagementWidgets/Documents/CreateFolderForm.tsx 50.00% 1 Missing ⚠️
...m/EngagementWidgets/Documents/UploadFileDrawer.tsx 50.00% 1 Missing ⚠️
...gagementWidgets/Events/InPersonEventFormDrawer.tsx 50.00% 1 Missing ⚠️
...agementWidgets/Events/VirtualSessionFormDrawer.tsx 50.00% 1 Missing ⚠️
...nts/engagement/form/EngagementWidgets/Map/Form.tsx 50.00% 1 Missing ⚠️
...ent/form/EngagementWidgets/WidgetDrawerContext.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2579      +/-   ##
==========================================
- Coverage   76.04%   76.03%   -0.01%     
==========================================
  Files         609      609              
  Lines       22083    22105      +22     
  Branches     1785     1797      +12     
==========================================
+ Hits        16793    16808      +15     
- Misses       5026     5033       +7     
  Partials      264      264              
Flag Coverage Δ
metweb 65.03% <73.07%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
met-api/src/met_api/models/widget.py 91.07% <ø> (ø)
met-api/src/met_api/schemas/widget.py 100.00% <ø> (ø)
...ts/engagement/form/EngagementWidgets/Poll/Form.tsx 81.13% <100.00%> (+0.36%) ⬆️
...ngagement/form/EngagementWidgets/Timeline/Form.tsx 70.71% <100.00%> (+0.42%) ⬆️
...s/engagement/form/EngagementWidgets/Video/Form.tsx 75.38% <100.00%> (+0.78%) ⬆️
...ementWidgets/WhoIsListening/WhoIsListeningForm.tsx 62.50% <ø> (ø)
...idgets/WhoIsListening/WhoIsListeningOptionCard.tsx 89.74% <100.00%> (+0.26%) ⬆️
...gement/form/EngagementWidgets/WidgetCardSwitch.tsx 18.36% <100.00%> (ø)
met-web/src/models/widget.tsx 100.00% <100.00%> (ø)
...c/services/widgetService/DocumentService/index.tsx 19.04% <ø> (ø)
... and 12 more

Copy link

sonarcloud bot commented Aug 22, 2024

@Baelx
Copy link
Collaborator Author

Baelx commented Aug 22, 2024

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

@Baelx Baelx merged commit be4394d into main Aug 22, 2024
14 of 15 checks passed
NatSquared pushed a commit that referenced this pull request Aug 23, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants