Skip to content

Conversation

@gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Jul 9, 2024

What are the relevant tickets?

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

Description (What does it do?)

This PR refactors ChannelPageSkeleton.tsx into two separate files; DefaultChannelSkeleton.tsx and UnitChannelSkeleton.tsx. The reason for this is that the layout of the Banner sections of the Unit channels is different enough that it was starting to become difficult to make changes to the Unit specific layout without affecting the other types of channels. This defines a clear separation so that Unit channels that have more extensive branding / styling can be customized without worrying about affecting the other types of channel pages. The use of BannerPage was replaced with simply using the newer Banner component. Slots were added to Banner to allow customization in the way the Unit channel pages need. Lastly the original problem noted in the issue above was addressed, which was just that the non-Unit channel page banners looked off from the design.

Screenshots (if appropriate):

image
image
image
image
image
image

How can this be tested?

  • Spin up mit-open on this branch
  • Ensure your departments, topics and units are populated
    • docker compose exec web ./manage.py update_offered_by
    • docker compose exec web ./manage.py update_topics
    • docker compose exec web ./manage.py update_departments_schools
  • Visit the home page at http://localhost:8062/
  • Click the icon to open the nav menu, then click "By Topic"
  • Select any of the topics and verify that the text in the banner does not shift and lines up with the listing page
  • Click the icon to open the menu again and click "By Provider"
  • Click any of the units on the listing page
  • Verify that the layout of the unit channel pages has not changed
  • Repeat this process on mobile from the home page step

@gumaerc gumaerc added the Needs Review An open Pull Request that is ready for review label Jul 9, 2024
@shanbady shanbady self-requested a review July 9, 2024 13:20
@shanbady shanbady self-assigned this Jul 9, 2024
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

Code changes look good. I did notice 2 things. Seems like the "manage widgets" link in the settings/gear tooltip isnt working. I checked on rc and it seems its also broken there.

The other thing i noticed in mobile view for certain pages (ones with longer breadcrumbs), the breadcrumb appears to break apart .
Screenshot 2024-07-09 at 10 33 27 AM

@shanbady shanbady added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jul 9, 2024
@shanbady
Copy link
Contributor

shanbady commented Jul 9, 2024

Code changes look good. I did notice 2 things. Seems like the "manage widgets" link in the settings/gear tooltip isnt working. I checked on rc and it seems its also broken there.

The other thing i noticed in mobile view for certain pages (ones with longer breadcrumbs), the breadcrumb appears to break apart . Screenshot 2024-07-09 at 10 33 27 AM

looks like there is a separate ticket for removing the widget settings button https://github.com/mitodl/hq/issues/4730

@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Jul 9, 2024
@gumaerc
Copy link
Contributor Author

gumaerc commented Jul 9, 2024

@shanbady @ChristopherChudzicki Thanks for your reviews, this is ready for another look now.

@gumaerc gumaerc force-pushed the cg/adjust-channel-detail-header branch from e89cf4b to de9681e Compare July 9, 2024 20:48
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

👍

@gumaerc gumaerc force-pushed the cg/adjust-channel-detail-header branch from de9681e to a75ba6b Compare July 11, 2024 13:28
@gumaerc gumaerc merged commit ed7dd68 into main Jul 11, 2024
This was referenced Jul 11, 2024
mbertrand pushed a commit that referenced this pull request Jul 15, 2024
* add DefaultChannelSkeleton and use it when the channel type is not unit

* use separate templates for units and other channel types

* remove usage of BannerPage

* align styles

* more fine tuning of the styles

* fix tests

* remove unnecessary page wrappers

* consolidate breadcrumb target info

* de-flake test

* move consolidated elements to ChannelPageSkeleton

* consolidate more elements

* flaky test fix #2

* rename Skeleton -> Template

* rename subHeader -> subheader

* add new banner stories

* fix issue with wrapping breadcrumbs on mobile
@rhysyngsun rhysyngsun deleted the cg/adjust-channel-detail-header branch February 7, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants