-
Notifications
You must be signed in to change notification settings - Fork 3
Design QA #2308
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
base: main
Are you sure you want to change the base?
Design QA #2308
Conversation
ac996b8
to
4c7dc4e
Compare
@@ -35,7 +36,6 @@ import { | |||
} from "api" | |||
import { useLearningResourcesSearch } from "api/hooks/learningResources" | |||
import { useAdminSearchParams } from "api/hooks/adminSearchParams" | |||
import { GridColumn, GridContainer } from "@/components/GridLayout/GridLayout" |
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.
The removal of these components is not particularly related to the other changes in this PR. However, I'd really like to delete these components and this is the most prominent usage.
Suggestion: Check there are no unintended layout changes by opening RC and this branch in browser tabs 1 and 2. Ensure same zoom and size (easy w/o console open). Tab between them. You should see some layout changes—mentioned as the items in this PR—but nothing major or not included in that list.
* The goal here is to ensure the icons: | ||
* - scale at same rate when container shrinks | ||
* - are hidden if container is too small | ||
* | ||
* NOTE: containerType: size enables the container query but requires setting | ||
* a width on the container. We are doing this anyway. |
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.
@mbilalmughal Regarding Academic and Professional Icon scaling: As described in the issue, they scale at different rates on RC as window size decreases and it doesn't look great.
Here are few options for addressing this issue. For now, (2) is implemented.
-
Scale at same rate. They get very small. Awkward. (The button border still exists, just isn't really showing up in the video.)
option_shrinking_icons.mov
-
Scale at same rate, but disappear after they get kinda small.
option_disappearing_icons.mov
-
Change the facet sidebar so that it has a fixed width of 300px, or a width that scales between 275px and 300px, but never goes below 300px.
-
Remove the icons all together. This is simplest.
IMO (3) would be a good solution. When the facet sidebar gets too narrow it affects usability IMO. That said, I don't think we should consider (3) for now. This change would cause some issues with the cards getting too narrow.
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.
@ChristopherChudzicki I believe what you’ve done on (2) is great, Removing the icon makes sense here. 👍
@ChristopherChudzicki Thanks chris, these updates looks good to me now 👍 |
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.
A few things look off pixel, though sometimes not sure if I'm looking at the latest in Figma.
display: "flex", | ||
alignItems: "center", | ||
gap: size === "small" ? "4px" : "8px", | ||
})) | ||
|
||
const Certificate = styled.div` | ||
padding: 2px 4px; |
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.
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.
padding-left: 0 !important; | ||
} | ||
` | ||
|
||
const MobileFilter = styled.div` | ||
${({ theme }) => theme.breakpoints.up("md")} { | ||
display: none; | ||
} | ||
|
||
color: ${({ theme }) => theme.custom.colors.darkGray2}; | ||
margin-top: 20px; |
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.
Figma has a margin-top of 16px (actually padding on the tabs), though the tab style is old - I might not be looking at the current design https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=4026-60992&m=dev

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.
Yeah, those tabs are an ancient design we never updated. @mbilalmughal IMO it would be good to align that with our current mobile tabs.
I should have linked to this word doc in the PR description (it's in the issue) but the spacing change here was just between Filter/Sort and the search results https://docs.google.com/document/d/1PyPhf-pbsxcDJKKmjzWoFV3dP_7KpCbvx_YHllOL180/edit?tab=t.0
@@ -96,7 +96,7 @@ export const Title: React.FC<TitleProps> = styled(Linkable)` | |||
${{ ...theme.typography.subtitle1 }} | |||
height: ${theme.typography.pxToRem(40)}; | |||
${theme.breakpoints.down("md")} { | |||
${{ ...theme.typography.subtitle2 }} | |||
${{ ...theme.typography.subtitle3 }} |
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.
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.
Hmmm, good catch. The figma designs do look better (particularly the alignment of bookmark button on mobile, IMO).
Because:
- @mbilalmughal has already approved the screenshots/choices in this PR / this issue wasn't mentioned in the design QA doc
- and the PR
is pretty big alreadyalready touches lots of design features,
I'd like to leave it out of this PR. However, I did make the change locally and I'm going to make a separate, dedicated PR for the change.
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.
@@ -50,6 +40,7 @@ const LogoContainer = styled.div({ | |||
height: "40px", |
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.
These are not horizontally centered at <md. Figma has a top and bottom padding of 24px (we have 34px and 14px).

https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=14674-72974&m=dev

display: "flex", | ||
flexDirection: "column", | ||
gap: "40px", | ||
margin: "80px 0", |
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.
We have some default margin top on the title, so should account for that in the 80pc top margin:

In Figma:

https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=2197-31369&m=dev
What are the relevant tickets?
https://github.com/mitodl/hq/issues/7572
Description (What does it do?)
Fixes several design discrepancies.
How can this be tested?
View the changed areas. See below.
Screenshots
1. Resource Card Certificate / Price Spacing
Now looks like:
/ 
Diff (bottom is new version):

2. Explore MIT Filter / Hero Filter
The order and icons are now the same. Hero icon for "Free" changed to pricetag.
3. Search page filter spacing updates (mobile), Mobile ListRowCard title size decrease
4. Unit Page Changes (mostly mobile):
Changes:
body1
body2
for count and text, two-tone cards,Mobile (right hand is extra narrow, svg shrinks now)

5. Search Page (and some channel pages)
Free checkbox is now centered:
BEFORE / AFTER
6. My Lists icon sizing
BEFORE / AFTER
/ 
7. Profile Page Font Sizing
No change. Handled in a separate PR. (#2304 )
8. Topic Page Spacing
Matches Figma ... Top/bottom spacing changed to be symmetric, 80px on desktop 32px on mobile:
Should otherwise look same as RC.
9. Departments Page Spacing
Matches Figma ... Top/bottom spacing changed to be symmetric, 80px on desktop 20px on mobile:
10. Search Display Filter Alignment
The first facet should now always align with the top of the search results:
BEFORE / AFTER
11. Search Page Facet Filter Text
Now Neue-haas-grotesk and dark gray 2.
BEFORE / AFTER
12. Academic and Professional Icon Scaling
See code comment for more details. Behavior updated so that the icons scale at same rate then disappear.