topic detail banner / subtopic logic revisions #1646

gumaerc · 2024-10-02T21:31:33Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5361

Description (What does it do?)

This PR moves some things around relating to topic channel pages:

Subtopics moved from channel page body to the banner
New ChannelPageTemplate component created to give topic channels their own template
Clicking on Subtopics to load their channel pages now shows "Related Topics" aka their siblings as Subtopics of the parent topic
Action buttons moved below the channel title
Various style changes

Screenshots (if appropriate):

How can this be tested?

Spin up this branch of mit-learn
Ensure that you have enough data backpopulated to have some topics and courses in those topics
Visit http://localhost:8062/topics
Click one of the top level topics that contain subtopics to visit its channel page
Verify that the subtopics display in the manner described above and on the issue at desktop and mobile resolutions
Click one of the subtopic chips
Ensure that the "Subtopics" title above the chips now says "Related Topics"
Verify that you can click the 3rd tier breadcrumb and go back to the parent topic

ChristopherChudzicki

I took a very quick look at this and noticed... Left two comments, but also noticed this:

Seems only to happen at some widths... Maybe between small and medium? Unsure.

ChristopherChudzicki · 2024-10-08T14:07:49Z

frontends/mit-learn/src/pages/ChannelPage/TopicChannelTemplate.tsx

+      currentHref={parentTopic?.channel_url}
+    />
+  ) : (
+    <BreadcrumbsInternal current={parentTopic?.name ?? ""} />


Suggestion: The nature of our django apis is that many of them return null when something is missing (rather than undefined).

I don't think it's worth fighting that. We could just make the base Breadcrumbs component accept null for current and currentHref (from a JS perspective I suspect null works fine already; it's just TS that is being mad here.)

ChristopherChudzicki · 2024-10-08T14:21:12Z

frontends/mit-learn/src/pages/ChannelPage/TopicChannelTemplate.tsx

+  alignItems: "end",
+  flexGrow: 0,
+  flexShrink: 0,
+  order: 2,


order is usually something we should avoid, since it's visual only and doesn't affect keyboard nav. And here I don't think it does anything:

This got copied over from DefaultChannelTemplate: https://github.com/mitodl/mit-open/blob/d9ed99aa0f15f528f2ddbf9a0f963cdccfafd9a6/frontends/mit-learn/src/pages/ChannelPage/DefaultChannelTemplate.tsx#L28

My best guess is that this was done to try and control the position of the channel controls container. I agree that it doesn't do anything and have removed it from both places.

shanbady · 2024-10-08T14:33:30Z

frontends/ol-components/src/components/Banner/Banner.tsx

    backgroundImage: backgroundDim
      ? `linear-gradient(rgba(0 0 0 / ${backgroundDim}%), rgba(0 0 0 / ${backgroundDim}%)), url('${backgroundUrl}')`
      : `url(${backgroundUrl})`,
-    backgroundSize: "cover",


I think the removal of the background size cover is causing the breakage at the particular width that chris had pointed out.

Yea, @mbilalmughal and I had a back and forth about the background. The 140% size rule was for ultrawide screens. I moved the rule to only apply to lg and above, and for everything else reset it to default with the position centered and top aligned.

It seems to me that whether the appropriate value is 140%, 120%, 180%, etc, is going to vary image by image. cover seems like a reasonable universal default, but a percentage doesn't to me.

I'd suggest if we really want this to be 140%, it should be set only on the topic page, and not site-wide.

Banner has a backgroundSize prop, though it seems currently unused. Maybe that would be a good place for this?

shanbady

One thing I noticed is that the currently active breadcrumb doesnt appear to dim on this branch (only seems to be happening with topics afaik).

production:

branch:

gumaerc · 2024-10-08T14:42:16Z

One thing I noticed is that the currently active breadcrumb doesnt appear to dim on this branch (only seems to be happening with topics afaik).

production: branch:

@shanbady This is because you are viewing a subtopic, and as part of this PR the subtopic shows the parent topic breadcrumbs. The last one is clickable to return to the parent topic.

gumaerc · 2024-10-08T20:21:30Z

@ChristopherChudzicki @shanbady This is ready for another look whenever y'all have a moment

concerns addressed

shanbady · 2024-10-09T13:01:15Z

@gumaerc dismissed my review. will leave the green light to @ChristopherChudzicki

ChristopherChudzicki · 2024-10-09T17:23:38Z

frontends/mit-learn/src/pages/ChannelPage/ChannelPage.test.tsx

+    const subTopicChannel = subTopicChannels[0]
+    const filteredSubTopics = subTopics?.results.filter(
+      (t) =>
+        t.name.replace(/\s/g, "-") !== subTopicChannel.name.replace(/\s/g, "-"),


Could be t.name !== subtopicChannel.title ... topic name and channel title are equivalent. (Topic model should use title, I guess. Oh well).

ChristopherChudzicki

This looks good. Some minor comments, and two requests (remove the unused filter in python code, and take another look at the backgroundSize thing).

ChristopherChudzicki · 2024-10-09T17:32:18Z

frontends/mit-learn/src/pages/ChannelPage/TopicChannelTemplate.tsx

+  return parentTopic?.channel_url ? (
+    <BreadcrumbsInternal
+      current={parentTopic?.name}
+      currentHref={parentTopic?.channel_url}
+    />
+  ) : (
+    <BreadcrumbsInternal current={parentTopic?.name} />
+  )
+}


Suggested change

return parentTopic?.channel_url ? (

<BreadcrumbsInternal

current={parentTopic?.name}

currentHref={parentTopic?.channel_url}

/>

) : (

<BreadcrumbsInternal current={parentTopic?.name} />

)

}

return (

<BreadcrumbsInternal

current={parentTopic?.name}

currentHref={parentTopic?.channel_url}

/>

)

ChristopherChudzicki · 2024-10-09T17:43:10Z

frontends/ol-components/src/components/Banner/Banner.tsx

    backgroundImage: backgroundDim
      ? `linear-gradient(rgba(0 0 0 / ${backgroundDim}%), rgba(0 0 0 / ${backgroundDim}%)), url('${backgroundUrl}')`
      : `url(${backgroundUrl})`,
-    backgroundSize: "cover",


It seems to me that whether the appropriate value is 140%, 120%, 180%, etc, is going to vary image by image. cover seems like a reasonable universal default, but a percentage doesn't to me.

I'd suggest if we really want this to be 140%, it should be set only on the topic page, and not site-wide.

Banner has a backgroundSize prop, though it seems currently unused. Maybe that would be a good place for this?

ChristopherChudzicki · 2024-10-09T17:46:06Z

learning_resources/filters.py

+    def filter_id(self, queryset, _, values):
+        """Filter by topic ID"""
+        return multi_or_filter(queryset, "id", values)
+


This can be removed, right?

gumaerc · 2024-10-09T19:08:16Z

It seems to me that whether the appropriate value is 140%, 120%, 180%, etc, is going to vary image by image. cover seems like a reasonable universal default, but a percentage doesn't to me.

I'd suggest if we really want this to be 140%, it should be set only on the topic page, and not site-wide.

Banner has a backgroundSize prop, though it seems currently unused. Maybe that would be a good place for this?

@mbilalmughal Wanted the 140% size rule not just for topics but for any banner that falls back to using the default "steps" image. From what I understand, we changed the image itself to display better on very large screens. The cover rule stretches the image horizontally too much and you lose all the detail in the steps. What I'll do is set the default background image fallback within the banner component itself and only apply this rule if the default image is being used.

gumaerc · 2024-10-09T19:27:53Z

@ChristopherChudzicki This is ready for another look, but read the comment above about the background image size rules.

…nnels query "related topics" aka the topic's siblings

for more information, see https://pre-commit.ci

…f, and only apply large and up 140% background size if the default image is being used

ChristopherChudzicki

👍

* Add location field to LearningResource and LearningResourceRun models (#1604) * Always delete past events during ETL, and filter them out from api results too just in case (#1660) * Release 0.21.1 * Release date for 0.21.1 * remove unnecessary padding adjustment on Input base styles (#1666) * remove unnecessary padding adjustment on Input base styles * adjust actual textarea padding to match Figma * Content File Score Adjustment (#1667) * Release 0.21.2 * Restore program letter intercept view (#1643) * Restore program letter intercept view * Generate schema * Fix test * fixing import * Revert "fixing import" This reverts commit cf56807. --------- Co-authored-by: shankar ambady <ambady@mit.edu> * Release date for 0.21.2 * topic detail banner / subtopic logic revisions (#1646) * add new darker chip variant * move subtopic display to the banner * remove max width on banner subtitle texts * query subtopics only on parent topic channels, and on child topic channels query "related topics" aka the topic's siblings * don't display the current topic in related topics * show parent topic breadcrumbs on child topic channel pages * fix tests * only do subtopics mocking if necessary * fix editchannelpage test * move action buttons under the title on mobile * fix test * add new default channel background image * "toopics" * fix background position * remove accidentally committed file * fix background size on mobile * remove unnecessary order css rule * fix banner background on mobile * fix breadcrumb props * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove unnecessary topic id filter * remove unnecessary breadcrumb fallback * set the default banner background image in the banner component itself, and only apply large and up 140% background size if the default image is being used * properly set default background image --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Update Yarn to v4.5.0 (#1624) * Update Yarn to v4.5.0 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Clear Cache on Deploy (#1668) * adding initial cache clear command * adding cache clear command to heroku release script * fix docstrings * removing invalid flag from clear cache command (#1675) * prevent featured course carousel from re-randomizing (#1673) * use skipFeatured when invalidating userlists / learning paths after update * add a new function for updating query data for user_list_parents and learning_path_parents on the featured courses list when either is edited without re-randomizing * fix learning path invalidations * fix test * Shuffling around where the search_update event is fired so it happens in more places (#1679) - Changed HeroSearch to use the SearchField component instead of SearchInput, so we get the event capture stuff - Updated SearchInput to not require setPage (for use on the homepage) - Moved facet events out of the SearchPage and into SearchDisplay so they get called on other pages that use it - Also trigger the facet events on more things - should also now get sorting changes, choosing between categories, and clearing facets * Reinstate background steps image --------- Co-authored-by: Matt Bertrand <mrbertrand@gmail.com> Co-authored-by: Doof <mitx-devops@mit.edu> Co-authored-by: Carey P Gumaer <gumaerc@mit.edu> Co-authored-by: Anastasia Beglova <abeglova@mit.edu> Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com> Co-authored-by: shankar ambady <ambady@mit.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Shankar Ambady <shanbady@gmail.com> Co-authored-by: James Kachel <james@jkachel.com>

* Merge `main` into `nextjs` (#1687) * Add location field to LearningResource and LearningResourceRun models (#1604) * Always delete past events during ETL, and filter them out from api results too just in case (#1660) * Release 0.21.1 * Release date for 0.21.1 * remove unnecessary padding adjustment on Input base styles (#1666) * remove unnecessary padding adjustment on Input base styles * adjust actual textarea padding to match Figma * Content File Score Adjustment (#1667) * Release 0.21.2 * Restore program letter intercept view (#1643) * Restore program letter intercept view * Generate schema * Fix test * fixing import * Revert "fixing import" This reverts commit cf56807. --------- Co-authored-by: shankar ambady <ambady@mit.edu> * Release date for 0.21.2 * topic detail banner / subtopic logic revisions (#1646) * add new darker chip variant * move subtopic display to the banner * remove max width on banner subtitle texts * query subtopics only on parent topic channels, and on child topic channels query "related topics" aka the topic's siblings * don't display the current topic in related topics * show parent topic breadcrumbs on child topic channel pages * fix tests * only do subtopics mocking if necessary * fix editchannelpage test * move action buttons under the title on mobile * fix test * add new default channel background image * "toopics" * fix background position * remove accidentally committed file * fix background size on mobile * remove unnecessary order css rule * fix banner background on mobile * fix breadcrumb props * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove unnecessary topic id filter * remove unnecessary breadcrumb fallback * set the default banner background image in the banner component itself, and only apply large and up 140% background size if the default image is being used * properly set default background image --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Update Yarn to v4.5.0 (#1624) * Update Yarn to v4.5.0 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Clear Cache on Deploy (#1668) * adding initial cache clear command * adding cache clear command to heroku release script * fix docstrings * removing invalid flag from clear cache command (#1675) * prevent featured course carousel from re-randomizing (#1673) * use skipFeatured when invalidating userlists / learning paths after update * add a new function for updating query data for user_list_parents and learning_path_parents on the featured courses list when either is edited without re-randomizing * fix learning path invalidations * fix test * Shuffling around where the search_update event is fired so it happens in more places (#1679) - Changed HeroSearch to use the SearchField component instead of SearchInput, so we get the event capture stuff - Updated SearchInput to not require setPage (for use on the homepage) - Moved facet events out of the SearchPage and into SearchDisplay so they get called on other pages that use it - Also trigger the facet events on more things - should also now get sorting changes, choosing between categories, and clearing facets * Reinstate background steps image --------- Co-authored-by: Matt Bertrand <mrbertrand@gmail.com> Co-authored-by: Doof <mitx-devops@mit.edu> Co-authored-by: Carey P Gumaer <gumaerc@mit.edu> Co-authored-by: Anastasia Beglova <abeglova@mit.edu> Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com> Co-authored-by: shankar ambady <ambady@mit.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Shankar Ambady <shanbady@gmail.com> Co-authored-by: James Kachel <james@jkachel.com> * restoring up and down chevrons (#1690) * Config to set cache control headers (#1700) * Middleware to set cache headers * Set headers in the config * Upgrade Next.js * Regex to exclude paths with a file extension to apply only to routes * Remove middleware file * Instruct to only store in shared cache (not browsers) * Copy yarn releases to the Docker container (fixes build) (#1703) * Copy yarn releases * Build the docker image on all push * NextJS - re-enable program letter tests (#1696) * fixing program letter template * re-enabling test * adding space * remove unnecessary webpack customizations (#1704) * Increase cache duration (#1705) * Increase cache durations * Update comment * Prod deployment. Add Posthog vars * NextJS Sentry Integration (#1701) * configure sentry, fix error pages * add env vars to CI * add global styles to global-error * TEMPORARY self destruct button * cleanup * cleanup error boundary, other misc cleanup * remove a few useless things * add a simple test i guess * remove intentional error triggers * Appzi env vars and Sentry config * Move deploy jobs to respective workflow. Docker build for dry run --------- Co-authored-by: Matt Bertrand <mrbertrand@gmail.com> Co-authored-by: Doof <mitx-devops@mit.edu> Co-authored-by: Carey P Gumaer <gumaerc@mit.edu> Co-authored-by: Anastasia Beglova <abeglova@mit.edu> Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com> Co-authored-by: shankar ambady <ambady@mit.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Shankar Ambady <shanbady@gmail.com> Co-authored-by: James Kachel <james@jkachel.com> Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com>

gumaerc added the Work in Progress label Oct 2, 2024

gumaerc force-pushed the cg/topic-detail-subtopic-revisions branch 2 times, most recently from c6d6536 to 331bf90 Compare October 7, 2024 14:46

gumaerc added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Oct 7, 2024

gumaerc changed the title ~~topic detail subtopic revisions~~ topic detail banner / subtopic logic revisions Oct 7, 2024

shanbady self-assigned this Oct 8, 2024

gumaerc force-pushed the cg/topic-detail-subtopic-revisions branch from a498b8a to 9f7a578 Compare October 8, 2024 13:34

ChristopherChudzicki reviewed Oct 8, 2024

View reviewed changes

shanbady reviewed Oct 8, 2024

View reviewed changes

shanbady previously requested changes Oct 8, 2024

View reviewed changes

gumaerc force-pushed the cg/topic-detail-subtopic-revisions branch from 6ba031f to f9a3cfa Compare October 8, 2024 15:46

gumaerc force-pushed the cg/topic-detail-subtopic-revisions branch from 3650225 to 7e49e40 Compare October 9, 2024 14:20

ChristopherChudzicki reviewed Oct 9, 2024

View reviewed changes

ChristopherChudzicki requested changes Oct 9, 2024

View reviewed changes

gumaerc assigned ChristopherChudzicki and unassigned shanbady Oct 9, 2024

gumaerc added 7 commits October 9, 2024 15:29

add new darker chip variant

3e9c643

move subtopic display to the banner

2e28f91

remove max width on banner subtitle texts

e7277f7

query subtopics only on parent topic channels, and on child topic cha…

6ad22eb

…nnels query "related topics" aka the topic's siblings

don't display the current topic in related topics

7f70bad

show parent topic breadcrumbs on child topic channel pages

6d6ee88

fix tests

9164376

gumaerc and others added 16 commits October 9, 2024 15:29

only do subtopics mocking if necessary

71ff509

fix editchannelpage test

b998fe9

move action buttons under the title on mobile

65f339c

fix test

43a43ea

add new default channel background image

abf9bd6

"toopics"

6dcc95c

fix background position

f8e5106

remove accidentally committed file

c71febb

fix background size on mobile

3d45b18

remove unnecessary order css rule

306bd00

fix banner background on mobile

2262c20

fix breadcrumb props

279b448

[pre-commit.ci] auto fixes from pre-commit.com hooks

6d7d433

for more information, see https://pre-commit.ci

remove unnecessary topic id filter

a0aa84c

remove unnecessary breadcrumb fallback

568dc45

set the default banner background image in the banner component itsel…

0dc711c

…f, and only apply large and up 140% background size if the default image is being used

gumaerc force-pushed the cg/topic-detail-subtopic-revisions branch from 9cd4239 to 0dc711c Compare October 9, 2024 19:29

properly set default background image

ad51130

ChristopherChudzicki approved these changes Oct 9, 2024

View reviewed changes

gumaerc merged commit b0cb1ea into main Oct 10, 2024
11 checks passed

This was referenced Oct 10, 2024

Release 0.21.3 #1674

Closed

Release 0.21.3 #1676

Merged

jonkafton mentioned this pull request Oct 14, 2024

Merge main into nextjs #1687

Merged

ChristopherChudzicki mentioned this pull request Oct 18, 2024

Merge main into nextjs #1708

Closed

rhysyngsun deleted the cg/topic-detail-subtopic-revisions branch February 7, 2025 20:37

topic detail banner / subtopic logic revisions #1646

topic detail banner / subtopic logic revisions #1646

Uh oh!

Conversation

gumaerc commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shanbady left a comment

Choose a reason for hiding this comment

Uh oh!

gumaerc commented Oct 8, 2024

Uh oh!

gumaerc commented Oct 8, 2024

Uh oh!

shanbady commented Oct 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumaerc commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gumaerc commented Oct 9, 2024

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gumaerc commented Oct 2, 2024 •

edited

Loading

gumaerc commented Oct 9, 2024 •

edited

Loading