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

SOEOPSFY24-22 #310

Merged
merged 5 commits into from
Apr 8, 2024
Merged

SOEOPSFY24-22 #310

merged 5 commits into from
Apr 8, 2024

Conversation

imonroe
Copy link
Contributor

@imonroe imonroe commented Dec 12, 2023

READY FOR REVIEW

Summary

  • When you have a List paragraph with an Event list, and it's set in Card Grid view mode, and you limit the output to fewer than three (or if the query returns fewer than three) items, the items are squished to the left of the paragraph, with a hole for empty space.
  • I'm proposing something like this CSS-only fix, but I'm not sure it's optimal.

Review By (Date)

  • When does this need to be reviewed by?

Criticality

  • 2

Urgency

  • normal

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Rebuild Cache
  3. Create 3 events, dated in the future.
  4. Create a page with a 2-column layout. In the first column, add a List paragraph. Set it to be an Event List, and choose card view. For the limit, set it to 3.
  5. observe the page, and note the card grid shows up correctly.
  6. In the paragraph, change the limit to 2.
  7. Observe the page, and note that only two items are shown, and they look right.
  8. In the paragraph, change the limit to 1. Note only one item shows up, and it looks right.

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory? NO.

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

  • JIRA ticket(s)
  • Other PRs
  • Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
  • Anyone who should be notified? (@mention them here)

Resources

@imonroe
Copy link
Contributor Author

imonroe commented Dec 12, 2023

Here is what it looks like with a limit set to one:

image

Here's what it looks like with a limit set to two:

image

Here's what it looks like with a limit set to three:

image

@imonroe imonroe requested a review from jenbreese December 12, 2023 20:41
@imonroe
Copy link
Contributor Author

imonroe commented Dec 12, 2023

@jenbreese Does this seem like a good solution to you? For context, see: https://stanfordits.atlassian.net/browse/SOEOPSFY24-22

@github-actions github-actions bot added the patch label Jan 8, 2024
@github-actions github-actions bot removed the patch label Jan 8, 2024
@imonroe imonroe requested a review from pookmish January 8, 2024 19:22
@pookmish
Copy link
Contributor

pookmish commented Jan 8, 2024

There is a trade off with this approach. If you choose to display more than 3 but only 4 items are available, the 4th item takes up the whole area. I think this is why we've been using grid more to keep them all the same regardless.

You could do some JS/template logic that adds/removes classes based on the number of elements. ie 1-3 use the flex, 4+ use grid. 🤷

@imonroe
Copy link
Contributor Author

imonroe commented Feb 22, 2024

Pinging @pookmish on this, as it sort of fell off my radar. Do we really want to build template logic as suggested above? My instinct is that building complex template logic to deal with numbers of items divisible by four is maybe overkill? The original ticket was an edge case, and having three columns and four items is another edge case. I'll rework it if necessary, but my feeling is that it might be overkill....

@github-actions github-actions bot added the patch label Apr 8, 2024
@imonroe
Copy link
Contributor Author

imonroe commented Apr 8, 2024

It did need to have a wrap added.

This is with five items.

image

This is with four items. After six, it wraps again.

image

@pookmish
Copy link
Contributor

pookmish commented Apr 8, 2024

Yeah, so that's the tradeoff with the flex. The 4th items with 4 total, 7th item with 7 total, etc, take up the whole space. Some think it's acceptable some don't like it. I'm personally in the latter because i don't like one event card to be more space consuming than others. But it's really up to the client/UX to make that choice. The actual approach in the PR is acceptable to me.

@imonroe
Copy link
Contributor Author

imonroe commented Apr 8, 2024

Well, @buttonwillowsix made the request originally, so I'll see if she'd like to review before merging.

@github-actions github-actions bot removed the patch label Apr 8, 2024
@buttonwillowsix
Copy link
Contributor

This looks great to me!

@pookmish
Copy link
Contributor

pookmish commented Apr 8, 2024

Good with her, good with me. Probably need to rebase/merge the lastest 11.x into this branch though

@imonroe imonroe merged commit 20b9b6e into 11.x Apr 8, 2024
4 checks passed
@imonroe imonroe deleted the SOEOPSFY24-22 branch April 8, 2024 22:01
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