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

SDSS-1201 | @jdwjdwjdw | Add 4 column card grid option for lists, adjust one-column layout, fixup load more issue #414

Merged
merged 14 commits into from
Jun 13, 2024

Conversation

jdwjdwjdw
Copy link
Contributor

@jdwjdwjdw jdwjdwjdw commented Apr 22, 2024

READY FOR REVIEW

Summary

  • SDSS-1201: FE/BE Developer: Option for 4 across in Lists
    • Added an option within lists which allows you to select a max of 4 grid columns. Lists will go to 4 columns above 1500px. When adding the list, make sure that the Items to display under Advanced options is set to something above 3.
    • As part of this work, I updated the config to allow us to directly target full-width pages (based on previous work done at https://github.com/SU-SWS/ppo_irsr_subtheme/tree/1.x/src/scss/basic-page) so that the widths will look good no matter which layout you select. Moving forward, if we need to target full-width basic pages specifically, we can add those styles within full-width.scss.
Screenshot 2024-04-24 at 1 42 08 PM

Screenshot 2024-04-24 at 1 53 20 PM

  • One column adjustments

    • While working on this PR I noticed some max-width issues with the one-column layout. I worked with the design team to get clarification on how all of the paragraphs should look for the one column layout - see related slack conversation. Per design specifications, one-column max-content area will be:
      • Text Area Paragraph: 850px
      • Card Paragraph: 980px
      • News List Card: 980px
      • Teaser: 980px (only if 1 item is displayed)
      • Banner, Image Gallery, List, Media with Caption, News Spotlight: 100% width
  • Added a fixup for an issue I discovered at https://sustainability.stanford.edu/news/school-news/all. If you click the Load More button for the News Items and then scroll to the top of the page, you can see that a new search icon is added for every time you click the Load More button.
    Screenshot 2024-04-24 at 1 45 13 PM

Review By (Date)

  • When convenient

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. For 4-column work: Add a basic page with a one column layout. Add a Card Grid list within the header of that layout (leaving other sections empty), and in the Styles/Behaviors section, select the Max 4 Grid Columns option under Max number of grid columns. Confirm that the list goes to 4-columns above 1500px, and the width look good for both a Full Width page, as well as a Layout: None page.
  3. For one-column work: test out all of the paragraphs in a one-column layout, for both Full Width and Layout: None pages. Confirm that all paragraphs look good and match the updated guidance from design
  4. For search bug: recreate https://sustainability.stanford.edu/news/school-news/all by cloning/publishing a lot of news items, then add a news list with Load More button. Confirm that clicking the Load more button no longer adds additional search links in the main navigation DOM.
  5. Review code 📚

Associated Issues and/or People

  • SDSS-1201: FE/BE Developer: Option for 4 across in Lists

@github-actions github-actions bot added the patch label May 7, 2024
@jdwjdwjdw
Copy link
Contributor Author

@jenbreese ready for another round of review. I made additional changes to the one-column layout per the related slack conversation with design.

@github-actions github-actions bot removed the patch label May 10, 2024
@jdwjdwjdw jdwjdwjdw changed the title SDSS-1201 | @jdwjdwjdw | Add 4 column card grid option for lists SDSS-1201 | @jdwjdwjdw | Add 4 column card grid option for lists, adjust one-column layout, fixup load more issue May 10, 2024
@jenbreese
Copy link
Contributor

@jdwjdwjdw Just a note that the author has to change the default number to display too or they will only get three.

@jenbreese
Copy link
Contributor

@jdwjdwjdw Everything looks good. The news list card on the 1-col does not fill to 980px. It looks like this for both layout none and full width.
Screenshot 2024-05-14 at 9 56 38 AM
Screenshot 2024-05-14 at 9 52 57 AM

@jdwjdwjdw
Copy link
Contributor Author

@jdwjdwjdw Everything looks good. The news list card on the 1-col does not fill to 980px. It looks like this for both layout none and full width. Screenshot 2024-05-14 at 9 56 38 AM Screenshot 2024-05-14 at 9 52 57 AM

Thanks @jenbreese! I asked design for clarification regarding the News List Card - they were referring to the News List Card paragraph type when they asked for that to be 980px. Design confirmed that list card grids with one or two items displayed are good as is - so we should be good there.

Copy link
Contributor

@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.

This looks great. I approve. Don't merge it yet until @joegl says it is ok. We are holding all merges until after the release tomorrow.

Copy link
Collaborator

@joegl joegl left a comment

Choose a reason for hiding this comment

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

Looks good! I have a couple concerns about content governance / design I left in the ticket and then a quick question about the conditions in one of the hooks.

@joegl
Copy link
Collaborator

joegl commented Jun 6, 2024

@jdwjdwjdw I merged the latest 4.x branch to resolve some merge conflicts and while testing on Gitpod I was unable to get the 4 column max grid to work.

I created a basic page with a 1-4-1 layout, added a list paragraph to the header, selected a card grid display and set the max 4 column grid setting. Can you double-check if anything changed that maybe stopped this from working correctly?

Here's a couple screenshots from my testing:
test-list-1
test-list-2
test-result

@jdwjdwjdw
Copy link
Contributor Author

@jdwjdwjdw I merged the latest 4.x branch to resolve some merge conflicts and while testing on Gitpod I was unable to get the 4 column max grid to work.

I created a basic page with a 1-4-1 layout, added a list paragraph to the header, selected a card grid display and set the max 4 column grid setting. Can you double-check if anything changed that maybe stopped this from working correctly?

@joegl can you try using a one-column layout instead of the 1-4-1? When I wrote the instructions we were using the 1-4-1 layout but once I made all of those max-width changes it enabled us to use the more preferable one-column layout. Apologies for any confusion, I didn't realize those instructions were out of date.

Copy link
Collaborator

@joegl joegl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Jacob!

@joegl joegl merged commit 17a4316 into 4.x Jun 13, 2024
5 checks passed
@joegl joegl deleted the SDSS-1201--4-col-lists branch June 13, 2024 16:06
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.

4 participants