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

Fix: modal for 4 or more agencies #2516

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Nov 8, 2024

Closes #2431

This PR fixes the Agency selector modal to arrange the agencies in the following manner:

  • 2 in the first row and 1 centered in the second row when 3 agencies are shown
    image
  • 2 in the first row and 2 in the second row when 4 agencies are shown
    image
  • 2 in the first row, 2 in the second row, and 1 centered in the third row when 5 agencies are shown
    image
  • all in one column in mobile view
    image

Reviewing

Replace your local TransitAgency fixtures to use (the data is incorrect, but we only need to review the modal)

 {
   "model": "core.transitagency",
   "pk": 1,
   "fields": {
     "active": true,
     "slug": "cst",
     "short_name": "CST (local)",
     "long_name": "California State Transit (local)",
     "info_url": "https://www.agency-website.com",
     "phone": "1-800-555-5555",
     "index_template": "core/index--cst.html",
     "eligibility_index_template": "eligibility/index--cst.html",
     "eligibility_api_id": "cst",
     "eligibility_api_private_key": 2,
     "eligibility_api_public_key": 3,
     "eligibility_api_jws_signing_alg": "RS256",
     "transit_processor": 1,
     "transit_processor_audience": "",
     "transit_processor_client_id": "",
     "transit_processor_client_secret_name": "cst-transit-processor-client-secret",
     "staff_group": 2,
     "customer_service_group": 2,
     "logo_large": "agencies/cst-lg.png",
     "logo_small": "agencies/cst-sm.png"
   }
 },
 {
   "model": "core.transitagency",
   "pk": 2,
   "fields": {
     "active": true,
     "slug": "mst",
     "short_name": "MST (local)",
     "long_name": "Monterey-Salinas Transit (local)",
     "info_url": "https://www.agency-website.com",
     "phone": "1-800-555-5555",
     "index_template": "core/index--cst.html",
     "eligibility_index_template": "eligibility/index--cst.html",
     "eligibility_api_id": "cst",
     "eligibility_api_private_key": 2,
     "eligibility_api_public_key": 3,
     "eligibility_api_jws_signing_alg": "RS256",
     "transit_processor": 1,
     "transit_processor_audience": "",
     "transit_processor_client_id": "",
     "transit_processor_client_secret_name": "cst-transit-processor-client-secret",
     "logo_large": "agencies/mst-lg.png",
     "logo_small": "agencies/mst-sm.png"
   }
 },
 {
   "model": "core.transitagency",
   "pk": 3,
   "fields": {
     "active": true,
     "slug": "sbmtd",
     "short_name": "SBMTD (local)",
     "long_name": "Santa Barbara MTD (local)",
     "info_url": "https://www.agency-website.com",
     "phone": "1-800-555-5555",
     "index_template": "core/index--cst.html",
     "eligibility_index_template": "eligibility/index--cst.html",
     "eligibility_api_id": "cst",
     "eligibility_api_private_key": 2,
     "eligibility_api_public_key": 3,
     "eligibility_api_jws_signing_alg": "RS256",
     "transit_processor": 1,
     "transit_processor_audience": "",
     "transit_processor_client_id": "",
     "transit_processor_client_secret_name": "cst-transit-processor-client-secret",
     "logo_large": "agencies/sbmtd-lg.png",
     "logo_small": "agencies/sbmtd-sm.png"
   }
 },
 {
   "model": "core.transitagency",
   "pk": 4,
   "fields": {
     "active": true,
     "slug": "sacrt",
     "short_name": "SacRT (local)",
     "long_name": "Sacramento Regional Transit (local)",
     "info_url": "https://www.agency-website.com",
     "phone": "1-800-555-5555",
     "index_template": "core/index--cst.html",
     "eligibility_index_template": "eligibility/index--cst.html",
     "eligibility_api_id": "cst",
     "eligibility_api_private_key": 2,
     "eligibility_api_public_key": 3,
     "eligibility_api_jws_signing_alg": "RS256",
     "transit_processor": 1,
     "transit_processor_audience": "",
     "transit_processor_client_id": "",
     "transit_processor_client_secret_name": "cst-transit-processor-client-secret",
     "logo_large": "agencies/sacrt-lg.png",
     "logo_small": "agencies/sacrt-sm.png"
   }
 },
 {
   "model": "core.transitagency",
   "pk": 5,
   "fields": {
     "active": true,
     "slug": "nevco",
     "short_name": "NevCo (local)",
     "long_name": "Nevada County Connects (local)",
     "info_url": "https://www.agency-website.com",
     "phone": "1-800-555-5555",
     "index_template": "core/index--cst.html",
     "eligibility_index_template": "eligibility/index--cst.html",
     "eligibility_api_id": "cst",
     "eligibility_api_private_key": 2,
     "eligibility_api_public_key": 3,
     "eligibility_api_jws_signing_alg": "RS256",
     "transit_processor": 1,
     "transit_processor_audience": "",
     "transit_processor_client_id": "",
     "transit_processor_client_secret_name": "cst-transit-processor-client-secret",
     "logo_large": "agencies/nevco-lg.png",
     "logo_small": "agencies/nevco-sm.png"
   }
 }

and use Django's template filter, for example, {% for agency in active_agencies|slice:"0:3" %} (where the slice goes from 0 (included) to 3 (not included)) in

to easily change the number of agencies being displayed in the modal.

@lalver1 lalver1 self-assigned this Nov 8, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Nov 8, 2024
Copy link

github-actions bot commented Nov 8, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@lalver1
Copy link
Member Author

lalver1 commented Nov 12, 2024

I am running into one problem still, I added

<h3 class="modal-title h1 p-0 mb-4 pb-lg-2s text-center"></h3>

because otherwise the cards in the mobile view are too tall, but this line is clearly unnecessary.

Comment on lines 699 to 703
.card-row {
gap: var(--card-gap);
.agency-container-width {
width: 75%;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be a Bootstrap utility class, unless it needs to change for a viewport width or something:

https://getbootstrap.com/docs/5.3/utilities/sizing/#relative-to-the-parent

w-75 adds

width: 75% !important;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @machikoyasuda!

Initially I was having trouble getting all the layouts to show up correctly using width: 75%; only so I tried out grid, but with

width: 75%;
margin: 0 auto;

as you wrote in the ticket makes all the layouts work. I'll update this PR shortly to keep using flex instead of grid.

@lalver1 lalver1 marked this pull request as ready for review November 19, 2024 22:31
@lalver1 lalver1 requested a review from a team as a code owner November 19, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Agency selector: Modal in case of 4 or more agencies
2 participants