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

YALB-363: Design: Serif text in right column gets cut off | YALB-365: Design: Grand Hero Banner overlay header spacing | YALB-424: Removing primary-nav and grand hero results in missing fontawesome #341

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

joetower
Copy link
Contributor

@joetower joetower commented Mar 12, 2024

YALB-363: Design: Serif text in right column gets cut off

YALB-365: Design: Grand Hero Banner overlay header spacing

YALB-424: Removing primary-nav and grand hero results in missing fontawesome

Description of work

  • YSP-363: Adds overflow override to text component in second column so the serif along the left side doesn't get cut off.
  • YSP-365: Adds conditional check for grand hero content so the markup for grand hero content doesn't render, causing the gap flex spacing to add more space when only the heading is present
  • YSP-424: Adds fontawesome library in atomic.yml file and removes the attach statements from block templates

Testing Link(s)

Functional Review Steps

Testing: YALB-363: Design: Serif text in right column gets cut off

  • Navigate to: https://pr-601-yalesites-platform.pantheonsite.io/profile
  • Scroll to the bottom of the content on the page to the two-column layout
  • Verify that the 'Y' serifs in Yale fully render and are not cut off by their parent text div
  • Note: this issue was caused by overflow-x: auto on the text element. This was added to allow table elements to scroll when users are on smaller screen.
ysp-363-serifs.mp4
  • Navigate to this page: https://pr-601-yalesites-platform.pantheonsite.io/table-page-test
  • If you view the page at mobile/smaller resolutions, verify the table still allows you to scroll.
  • I added some JS to wrap any table element in a wrapper div, which has the overflow-x: auto property so we don't have to set it on the text element and cause other issues.
ysp-363-serifs-table-wrapper.mp4

Testing: YALB-365: Design: Grand Hero Banner overlay header spacing

  • Navigate to: https://pr-601-yalesites-platform.pantheonsite.io/profile
  • Verify that the hero heading is the only content in the hero content area
  • Note: This issue was caused by the content div element (grand-hero-banner__snippet) printing to the DOM even if no content was entered and the grand-hero-banner__content-inner element has a gap property set which added extra space in between the heading and the empty content div. I changed the wiring in Atomic to conditionally display the value if a value actually exists. With the empty content markup no longer printing, the heading does not have extra space. I also adjusted the bottom padding to match the top padding. Previously, the bottom padding was using a larger padding variable than the top.
  • Inspect the heading and verify there is equal space around the heading and the content appears vertically centered within its background
ysp-365-hero.mp4

Testing: YALB-424: Removing primary-nav and grand hero results in missing fontawesome

ysp-424-fontawesome.mp4

Accessibility Review

  • Verify the component meets Accessibility requirements

@joetower joetower self-assigned this Mar 12, 2024
Copy link

netlify bot commented Mar 12, 2024

Deploy Preview for dev-component-library-twig ready!

Name Link
🔨 Latest commit b38c4cb
🔍 Latest deploy log https://app.netlify.com/sites/dev-component-library-twig/deploys/65fd8503be3f1e0008a36bab
😎 Deploy Preview https://deploy-preview-341--dev-component-library-twig.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joetower joetower marked this pull request as ready for review March 13, 2024 15:44
Copy link
Contributor

@codechefmarc codechefmarc left a comment

Choose a reason for hiding this comment

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

Tested all of these out and all looks good to me! I didn't technically test the font-awesome one (I can if you wish) but I didn't want to remove the main menu from the dev site to fully test it, but I understand what you did to load FA on all pages which makes sense.

Copy link
Contributor

@dblanken-yale dblanken-yale left a comment

Choose a reason for hiding this comment

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

There is unfortunately one more place to fix the sr-only, and that's in the JS creation in case we ever miss the twig generation:

Other than that, it all looks great! Due to such a trivial fix above, I'm still approving. Nicely done!

@joetower joetower changed the title feat(YSP-365): conditional display of content to remove gap between t… YALB-363: Design: Serif text in right column gets cut off | YALB-365: Design: Grand Hero Banner overlay header spacing | YALB-424: Removing primary-nav and grand hero results in missing fontawesome Mar 13, 2024
@joetower
Copy link
Contributor Author

@dblanken-yale Ah, great catch, thank you!

I updated that class name.

Copy link

@kara-franco kara-franco left a comment

Choose a reason for hiding this comment

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

Nice work! Passes a11y tests!

@nJim nJim merged commit ad66ec7 into develop Mar 22, 2024
5 checks passed
@nJim nJim deleted the YSP-363-365-426-bugs branch March 22, 2024 13:25
@vinmassaro vinmassaro mentioned this pull request Mar 22, 2024
Copy link

🎉 This PR is included in version 1.40.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants