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

Feature/deseng666 Add new Engagement Details page #2574

Merged
merged 5 commits into from
Aug 13, 2024
Merged

Conversation

Baelx
Copy link
Collaborator

@Baelx Baelx commented Aug 9, 2024

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

Description of changes:

  • Add new engagement details page
  • Update folder structure
  • Preserve old engagement view page under "old-view" URL
  • Update autobreadcrumbs to use the "nav" element by default
  • Change sidenav admin menu for increased design parity

**You can ignore anything under the old-view or public directories. Git didn't understand that I just wanted to move things around.

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.03%. Comparing base (75aed45) to head (a7d5371).

Files Patch % Lines
...eb/src/components/common/Navigation/Breadcrumb.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2574   +/-   ##
=======================================
  Coverage   76.03%   76.03%           
=======================================
  Files         609      609           
  Lines       22075    22076    +1     
  Branches     1782     1783    +1     
=======================================
+ Hits        16785    16786    +1     
  Misses       5026     5026           
  Partials      264      264           
Flag Coverage Δ
metweb 65.01% <85.71%> (+<0.01%) ⬆️

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

Files Coverage Δ
met-web/src/components/banner/Banner.tsx 100.00% <ø> (ø)
met-web/src/components/banner/BannerWithImage.tsx 84.61% <ø> (ø)
...t-web/src/components/banner/BannerWithoutImage.tsx 100.00% <ø> (ø)
...c/components/engagement/old-view/ActionContext.tsx 60.60% <ø> (ø)
.../src/components/engagement/old-view/EmailModal.tsx 53.84% <ø> (ø)
.../src/components/engagement/old-view/EmailPanel.tsx 31.57% <ø> (ø)
...gement/old-view/EngagementBanner/BannerSection.tsx 100.00% <ø> (ø)
...nts/engagement/old-view/EngagementBanner/index.tsx 100.00% <ø> (ø)
...mponents/engagement/old-view/EngagementContent.tsx 100.00% <ø> (ø)
...ents/engagement/old-view/EngagementInfoSection.tsx 95.65% <ø> (ø)
... and 28 more

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.

Looks good; thanks! Since you're away today I'll patch some errors and get this merged for you

met-web/src/components/engagement/admin/view/index.tsx Outdated Show resolved Hide resolved
return data.engagement.then((engagement) => {
return {
link: `/engagements/${engagement.id}/view`,
name: engagement.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done with the breadcrumb function :)

<Breadcrumbs aria-label="breadcrumb" sx={smallScreenOnly ? { display: { xs: 'block', md: 'none' } } : {}}>
<Breadcrumbs
aria-label="breadcrumb"
component="nav"
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for making this a11y improvement :3

@NatSquared NatSquared self-requested a review August 9, 2024 19:30
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.

Sorry, I just realized - all the components in components/engagement/old-view are unused and have no references to them, so they are redundant. All the components are still present in components/engagement/view since they were copied and not renamed. I'd like to discuss this with you before anything gets merged.

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.

Thanks for making sure those files were moved correctly - approved!

Copy link

sonarcloud bot commented Aug 13, 2024

@Baelx Baelx merged commit 9ace9ec into main Aug 13, 2024
8 checks passed
@Baelx Baelx deleted the feature/deseng666 branch August 13, 2024 18:38
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