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

CHEMH-83 | @jdwjdwjdw | Structure and style News node page #67

Merged
merged 15 commits into from
Sep 29, 2023

Conversation

jdwjdwjdw
Copy link
Contributor

@jdwjdwjdw jdwjdwjdw commented Aug 30, 2023

READY FOR REVIEW

Summary

  • See News node page in figma
  • CHEMH-83: Style News Node
    • wasn't able to match designs completely due to drupal/subtheme limitations, but got as close as I could.
  • CHEMH-242: Weird treatment on load more button post Layout Paragraphs

Review By (Date)

  • When convenient

Urgency

  • Normal

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Create an example News item, and navigate to the News node page
  3. Confirm that the news node page matches figma (outside of limitations with field order/display)
  4. Confirm that the See All News button at the bottom of the news node page looks correct and fixes the issue mentioned in CHEMH-242
  5. Review code 📰

Associated Issues and/or People

  • CHEMH-83: Style News Node
  • CHEMH-242: Weird treatment on load more button post Layout Paragraphs

Copy link
Collaborator

@jenbreese jenbreese left a comment

Choose a reason for hiding this comment

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

There's a lot of extra space after the more news cards for both the responsive and desktop sizes.
Screenshot 2023-09-26 at 9 40 14 AM
Screenshot 2023-09-26 at 9 39 54 AM

@jdwjdwjdw
Copy link
Contributor Author

There's a lot of extra space after the more news cards for both the responsive and desktop sizes. Screenshot 2023-09-26 at 9 40 14 AM Screenshot 2023-09-26 at 9 39 54 AM

Thanks @jenbreese! I fixed that up and addressed some other spacing issues in situations where there isn't a dek / a top banner is/isn't used. Should be good to re-review now.


.main-region {
@include grid-media-min('sm') {
-webkit-box-flex: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdwjdwjdw I'm don't think you need to have the -webkit prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@@ -136,36 +136,26 @@

.main-region {
@include grid-media-min('sm') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdwjdwjdw Can we discuss these media queries? When I'm looking at the results in the browser, only the one on line 138 is in effect, all the later ones are overridden.

@jdwjdwjdw jdwjdwjdw merged commit 57ed686 into 2.x Sep 29, 2023
1 check passed
@jdwjdwjdw jdwjdwjdw deleted the CHEMH-83--news-node branch September 29, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants