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

Link to teacher emails from teacher features #1231

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Conversation

axlewin
Copy link
Contributor

@axlewin axlewin commented Nov 21, 2024

No description provided.

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.63%. Comparing base (769fa7a) to head (7795bcb).
Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1231   +/-   ##
=======================================
  Coverage   36.63%   36.63%           
=======================================
  Files         447      447           
  Lines       19666    19666           
  Branches     6475     6475           
=======================================
  Hits         7205     7205           
  Misses      11816    11816           
  Partials      645      645           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

<IsaacCard doc={{ clickUrl: "/pages/isaac_embedded_schools",
image: {src: "/assets/phy/icons/teacher_features_sprite.svg#groups"},
title: "Teacher Ambassadors",
<IsaacCard doc={{ clickUrl: isTeacherOrAbove(user) ? "/teacher_emails" : "/pages/contact_us_teacher",
Copy link
Member

Choose a reason for hiding this comment

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

Jacob is right, you don't need the conditional here. But only because there's an easier way to implement the route that will give you the conditional redirect for free.

@@ -138,6 +139,7 @@ export const RoutesPhy = [
<StaticPageRoute key={key++} exact path="/alevel" pageId="alevel" />,
<TrackedRoute key={key++} exact path="/teacher_features" component={TeacherFeatures}/>,
<TrackedRoute key={key++} exact path="/tutor_features" component={TutorFeatures}/>,
<TrackedRoute key={key++} exact path="/teacher_emails" component={TeacherEmails}/>,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making a new component and making a new route like this, you could instead add it to the Teacher Pages bit below using <StaticPageRoute> like the mentoring pages.

I would also like it if we could force the fragment ID to be teacher_emails_frag so that it is more unique. Then the pageId you need for the route would be fragments/teacher_emails_frag.

@jacbn jacbn merged commit 4c19969 into master Nov 25, 2024
4 checks passed
@jacbn jacbn deleted the hotfix/teacher-emails branch November 25, 2024 11:33
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