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-670: Admin authoring header page #2593

Merged
merged 10 commits into from
Sep 24, 2024

Conversation

NatSquared
Copy link
Contributor

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

Description of changes:

  • Feature New Hero Banner page in authoring section
    • Hooks into existing AuthoringTemplate component for form submission
    • Added new fields for hero banner content
    • Added image upload functionality
    • Added layout components for engagement authoring form fields

User Guide update ticket (if applicable):
https://citz-gdx.atlassian.net/browse/DESENG-712

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 61.45833% with 37 lines in your changes missing coverage. Please review.

Project coverage is 75.13%. Comparing base (f51812c) to head (7a2c21d).

Files with missing lines Patch % Lines
met-api/src/met_api/services/engagement_service.py 44.44% 20 Missing ⚠️
...eb/src/components/common/Navigation/Breadcrumb.tsx 50.00% 9 Missing ⚠️
...t-web/src/components/common/Typography/Headers.tsx 16.66% 5 Missing ⚠️
...gement/admin/create/authoring/AuthoringSideNav.tsx 0.00% 2 Missing ⚠️
met-web/src/components/common/Input/FormField.tsx 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2593   +/-   ##
=======================================
  Coverage   75.12%   75.13%           
=======================================
  Files         612      612           
  Lines       22215    22244   +29     
  Branches     1859     1866    +7     
=======================================
+ Hits        16689    16712   +23     
- Misses       5253     5259    +6     
  Partials      273      273           
Flag Coverage Δ
metapi 87.12% <62.26%> (-0.03%) ⬇️
metweb 64.15% <60.46%> (+0.03%) ⬆️

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

Files with missing lines Coverage Δ
met-api/src/met_api/constants/engagement_status.py 100.00% <100.00%> (ø)
met-api/src/met_api/models/engagement.py 78.75% <ø> (-0.22%) ⬇️
...-api/src/met_api/models/engagement_status_block.py 100.00% <100.00%> (ø)
...t-api/src/met_api/models/engagement_translation.py 96.92% <ø> (-0.05%) ⬇️
met-api/src/met_api/schemas/engagement.py 93.22% <ø> (-0.23%) ⬇️
...api/src/met_api/schemas/engagement_status_block.py 100.00% <100.00%> (ø)
...-api/src/met_api/schemas/engagement_translation.py 100.00% <ø> (ø)
...met_api/services/engagement_translation_service.py 88.88% <ø> (-0.12%) ⬇️
met-web/src/components/common/Input/TextInput.tsx 100.00% <100.00%> (ø)
met-web/src/components/common/Typography/Body.tsx 85.18% <100.00%> (+4.23%) ⬆️
... and 13 more

Copy link
Collaborator

@Baelx Baelx left a comment

Choose a reason for hiding this comment

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

Thanks for some overall improvements and for your work! Please have a look at all my comments. To proceed, could you just:

  • Remove commented-out code
  • Document some of the new shared components to we're all in ✨alignment

for engagement in connection.execute(engagement_table.select()):
# Update existing engagement_status_blocks...
# If the URL starts with "http", it's an external link.
if engagement.cta_url and engagement.cta_url.startswith("http"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing the work of updating each status block!

I feel like we may miss some URLs if folks omit the protocol, but it's pretty low stakes at this point since there are no live engagements or anything.

@@ -119,6 +119,7 @@ const clearInputButton = (onClick: () => void) => {

export type TextFieldProps = {
error?: string;
errorPosition?: 'top' | 'bottom';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love exact typing for definite primitive values instead of just string, number, etc.!

.rdw-editor-wrapper:has(.public-DraftEditor-content[contenteditable="true"]){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding in these styles for richtexteditors in the app. No matter the designs, Steve would be very happy for us to design one RichTextEditor's look and have all of them that way.

Could I ask you to add a note about this this to Contributing under a new section called "Styling" if we don't already have one?

@@ -47,6 +50,21 @@ export const BodyText = ({
);
};

export const ErrorMessage = ({ error }: { error?: string }) => {
if (!error) {
return <></>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just return null and Typescript should be happy!

Suggested change
return <></>;
if (!error) return null;

</BodyText>
);
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this component would be for errors on form fields? Could you also capture in Contributing?

fetcher.data = undefined;
}
}, [fetcher.data]);
const pageName = useMatch('/engagements/:engagementId/details/authoring/:page')?.params.page;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe let's leave a comment somewhere that we need to only use the yup validation on this page and why we're doing so?

return (
<Grid item sx={{ maxWidth: '700px' }} direction="column">
{createPortal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We chatted a bit about using a React Portal here. Could I ask you to just remove all changes to this file? It's just changed so much right now that a lot of this isn't relevant anymore. I'll just make a note to use the Portal but if you could reset everything that would be great!

@@ -9,7 +9,7 @@ import { Palette } from 'styles/Theme';

const AuthoringFeedback = () => {
const [sectionHeading, setSectionHeading] = useState('');
const [bodyText, setBodyText] = useState('');
// const [bodyText, setBodyText] = useState('');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still need to be here?

setEditorState(newEditorState);
setBodyText(plainText);
// const plainText = newEditorState.getCurrentContent().getPlainText();
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀

import { Grid, GridProps, Box, BoxProps } from '@mui/material';
import { BodyText } from 'components/common/Typography';

export const AuthoringFormContainer = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not something to put into Contributing since we probably won't be adding new Authoring sections in the long-term but would be great to mention at our next group planning session!

Copy link

sonarcloud bot commented Sep 24, 2024

@NatSquared NatSquared merged commit b95a8c7 into main Sep 24, 2024
15 checks passed
@NatSquared NatSquared deleted the DESENG-670-admin-authoring-header-page branch September 24, 2024 23:44
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