-
Notifications
You must be signed in to change notification settings - Fork 1
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: update existing sections #489
Conversation
✅ Deploy Preview for cal-itp-mobility-marketplace ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I finished implementing the items that are in scope for #467. Something that needs clarification @segacy1 @thekaveman: the spacing on the current site and the spacing shown in Figma do not match. Is this an intentional design update that we should be implementing as a part of this homepage effort? The current spacing is using some odd values, and I don't know where these came from; they must've been based on something though, right? They were added just a few months ago. The spacing in Figma looks to me like it's based off the 8-pt system, so it seems more aligned with how we've said we want to do spacing. This change should be its own ticket if it's confirmed that we're supposed to be implementing this. |
Hi @angela-tran! Yes, this does follow the 8-pt grid. Whenever I create new designs for mobimart or calitp.org, I automatically update with that design framework. I know we never discussed that explicitly for the homepage dev work though—I'll do a better job noting that when we're scoping in the future. If we can create a new ticket for updating the spacing in backlog that would be great Also, I think you might be looking at my design sandbox. The design for implementation is here. |
Thank you @segacy1 — that is all very very helpful context Re:
This sounds good to me
Oh yes, you're right that I was looking at a page under "DESIGN_____". I was looking there because it's linked from the description for #463 (comment). Thanks for pointing this out. I can reference the one you linked |
84d64ff
to
ba5ed88
Compare
In Figma, I see that the content for the lower sections ("What is Cal-ITP?" and "Get in touch") are more or less vertically The current CSS that we use to lay out these sections are I have ideas on how to do it with Bootstrap's grid, which is really using CSS Flexbox, but the |
Or another thought: is the current spacing good enough for now until we take #490? |
Yeah I'm OK to wait to handle spacing all at once. Though I wonder how hard it would be in this PR to align the hero image with the two section images (or make the hero image slightly smaller to match)? They are just a bit off: |
This was a good catch, and the change for it was pretty straightforward: b6f1677 That style rule was from a much older version of the design (#275), so I think it's fine to just remove it |
Hmm @angela-tran I think we may have gone slightly the other way now? The two section images are larger (square) than the hero one now (same width, smaller height): |
@angela-tran Haven't had a chance to really dig into this but:
So one way to approach the image width/column horizontal spacing issue is to re-export the images with the white space, make sure the images are the same dimensions and stick it in the somewhat overeengineered CSS Grid situation that Mobi Mart already has. |
@machikoyasuda I came to the same conclusion and re-exported the images from Figma. Glad we were thinking the same |
@mrose914 can you please verify copy changes on the homepage (does not include the new sections, just edits to existing copy): https://deploy-preview-489--cal-itp-mobility-marketplace.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and visuals LGTM
Sorry, I just noticed there are some things in the mobile view that need to be fixed. - Fixed in 1645fc0
|
@machikoyasuda I went ahead and made this change in a1441ca |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on the mobile titles. Looks like we need one more small change on mobile for centering the images:
In the browser devtools I changed the following and it worked:
.imaged-section__image {
justify-content: space-around;
}
This also looks OK on the normal view so maybe we just add this style in?
@thekaveman Sounds good to me, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Closes #467
Top section
Lower sections