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

Refactor: agency logo width and height #1632

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Aug 9, 2023

Part of #1588

This PR updates the agency selector to use the new width and height that product and design came up with so that all logos fit within them. This PR also updates the MST and SacRT logos to use the files that design created to have correct padding.

(This PR previously refactored the modal to use separate templates for each agency, which prompted a lot of the discussion below.)

@angela-tran angela-tran added this to the SBMTD milestone Aug 9, 2023
@angela-tran angela-tran requested a review from a team as a code owner August 9, 2023 21:58
@angela-tran angela-tran self-assigned this Aug 9, 2023
@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 back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates tests Related to automated testing (unit, UI, integration, etc.) labels Aug 9, 2023
@machikoyasuda
Copy link
Member

image The SacRT is not centered on horizontally Tablet/Mobile anymore.

@angela-tran
Copy link
Member Author

The SacRT is not centered on horizontally Tablet/Mobile anymore.

Ah, thanks for catching that. It seems like we're basically relying on the widths being equal and using the largest logo's width to center them. So should we set SacRT back to the MST width? And when we add SBMTD's logo, which is the widest, we'll have to set MST and SacRT's logo widths to that one?

Or do you think we should do something in CSS to make them centered?

@machikoyasuda
Copy link
Member

machikoyasuda commented Aug 9, 2023

@angela-tran I think actually, Design should be giving us custom logo files for both Desktop and Mobile, with extra padding if it's a logo like SacRT's - which is narrower than MST/SBMTD. Like this Figma export has 9px on the left and right:
image

I think if Design always customized logos for Dev, and resized/centered/cropped/whatever-is-necessary to get all agency logos to fit 1 dimension - then we won't need this code at all. It would be Design's responsibility to come up with the size of the logo and how the logo is centered within a specified rectangular space that never changes for Desktop and Mobile. In other words, Devs should never go to an agency's site or an email or Google Drive to get an agency's logo file and upload that for use on the app -- this should always go through Design and Design needs to be resizing, cropping and centering the logo for use on the agency selector page for Desktop and Mobile.

cc @indexing @srhhnry

@angela-tran
Copy link
Member Author

angela-tran commented Aug 9, 2023

@machikoyasuda That makes sense to me and aligns I think with what @indexing and @srhhnry have been talking about with figuring out max-dimensions/character lengths for the Agency Card component.

In other words, Devs should never go to an agency's site or an email or Google Drive to get an agency's logo file and upload that for use on the app -- this should always go through Design and Design needs to be resizing, cropping and centering the logo for use on the agency selector page for Desktop and Mobile.

I agree that this would be a good process to follow going forward. I don't think we (product, design, and dev) ever discussed that this is the expectation. Do we block this until product/design gets us logos they've edited?

@angela-tran
Copy link
Member Author

angela-tran commented Aug 9, 2023

Update: Machiko let me know that Andy has already edited the logos in Figma and that they can be exported from there for use in this PR and #1631.

From Machiko:

The new design will have standard width/height for all logos, for Desktop and Mobile.
It'll be 148x72 for D and 60x48 for M for all agencies

Screenshots

image image

Looks like those screenshots were from a Figma board called "Copy Sync Demo" so I don't think these values are the ones that design intends for us to go with. Seems like the ones Sarah posted in #1632 (comment) are the ones currently.

@angela-tran angela-tran marked this pull request as draft August 9, 2023 22:52
@machikoyasuda
Copy link
Member

Awaiting completion of #1444

@srhhnry
Copy link
Member

srhhnry commented Aug 9, 2023

Hey! So per issue #1444 we've worked out a single dimension for mobile and desktop logos. The summary of that issue thread is that agency logos can be different shapes. Also, to make the Figma component work in a standardized way I have to manipulate the shape a bit (long story, but "hugging" components to make them look good breaks their fixed width/height--this might be a me problem).

Anyway, you can find the dimensions on the components page under Icons + Logos or as seen here:

image

Let me know if that works for you all, or if we need to adjust the sizing/dimension/approach

@machikoyasuda
Copy link
Member

@srhhnry We're gonna need 2 logo files for each agency, with the agency's logo files cropped to those now standard dimensions.

The desktop logos in the styleguide aren't exportable in the rectangular size. There's no layer with the file in it in that dimension.
image

The mobile logos are off by 1px
image
image
Should be 52px, not 51px.

If you could upload those files to the GitHub ticket or correct it on Figma, we just need the files in the correct dimensions one way or another.

@indexing
Copy link
Member

@srhhnry While there are a likely a variety of approaches to get agency assets for dev that export at a consistent dimension, I found this structure works.

  1. create an agency asset component for the base region at the spec size. (fixed dimensions/no auto layout)
  2. add variants for each agency
  3. remove fill and type, then size each agency logo to fit within the standard dimension in each variant.
  4. center agency logo vertically and horizontally

Since the logo and the region are fixed, there is no need for auto layout. (Auto layout might, however, be useful at the card level when nesting the logo region component in the agency selector card component even though its dimensions remain fixed.)

Screenshot 2023-08-09 at 5 29 00 PM

https://www.figma.com/file/SeSd3LaLd6WkbEYhmtKpO3/Benefits-(Full-Application)?type=design&node-id=11564%3A54870&mode=design&t=4laAn2Axfg0u6Mau-1

@srhhnry
Copy link
Member

srhhnry commented Aug 10, 2023

Per @indexing's suggestion on approach I added the mobile files to Figma as frames that conform to the standard sizing. So both. the desktop sizing Andy did as well as the mobile sizing is now complete. You can find them on the Components page and they look like this:
image

@machikoyasuda
Copy link
Member

@srhhnry Yay thank you. Can you do SBMTD logo too? We will need that for this upcoming PR. @angela-tran is getting ready to add SBMTD to the app.

@srhhnry
Copy link
Member

srhhnry commented Aug 10, 2023

I actually don't have the official logo from SBMTD (and am not sure where we are in the intake process with them) @o-ram do you know if we've gone through that questionnaire with SBMTD and if so, did they send over their official logo? Otherwise we're just pulling the image from their website, which might be fine but we'd want them to weigh in on it at least.

Edit: Nevermind @o-ram!

@machikoyasuda
Copy link
Member

@srhhnry It's here https://drive.google.com/file/d/1YG7SU7YObpOEWqYSBcOHPO1I79byPhjd/view

from here #1588 (comment)

@srhhnry
Copy link
Member

srhhnry commented Aug 10, 2023

Updated!

add new logo files from design that have correct dimensions
@angela-tran angela-tran changed the title Refactor: agency logo templates Refactor: agency logo width and height Aug 11, 2023
@angela-tran
Copy link
Member Author

Exported new logo files from Figma with same dimensions for all. The logos are centered:

image

@angela-tran angela-tran marked this pull request as ready for review August 11, 2023 01:01
@angela-tran angela-tran removed tests Related to automated testing (unit, UI, integration, etc.) back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates labels Aug 11, 2023
@angela-tran angela-tran merged commit 2b2ee37 into dev Aug 11, 2023
8 checks passed
@angela-tran angela-tran deleted the refactor/agency-logo-template branch August 11, 2023 01:26
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.

None yet

4 participants