-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add startups sidebar to partners #2688
Conversation
77a8d37
to
34022fc
Compare
34022fc
to
05c4b4e
Compare
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 might need some css tweaks here... I don't think the sidebar card is responsive like the other cards, so it feels a bit disjointed when it is close to other cards (probably needs a margin too):
Also may the filters to vertically stack or allow overflow below 350ish:
I'm also wondering about the placement, when you have more filters active, it can feel like it's floating out there...
@@ -202,13 +200,51 @@ function resultEntry(pd: PartnerData) { | |||
cost: pd.affordability_cost, | |||
rating: pd.average_rating.rating__avg, | |||
ratingCount: pd.rating_count, | |||
partnershipLevel: pd.partnership_level, | |||
partnershipLevel: pd.partnership_level as string, |
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.
I think it's already typed as string here https://github.com/openstax/os-webview/blob/add-startups-sidebar-to-partners/src/app/pages/partners/results/results.tsx#L52
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.
I made startups display as a normal grid-group if it's there is only one other grid-group.
I think I've got the spacing all fixed up.
The "as string" was leftover from when I had it defined as string | null. Good catch, thanks.
I removed some padding on the mobile-controls row, which was enough to make it good down to 320px screen width.
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 this is a lot better, nice idea to switch up the display based on the available groups.
@@ -15,6 +15,9 @@ import {treatSpaceOrEnterAsClick} from '~/helpers/events'; | |||
import './main-menu.scss'; | |||
|
|||
function DropdownOrMenuItem({item}) { | |||
if (! item.name && ! item.label) { |
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.
Some spaces here?
CORE-609
I have added a phone-format layout. On screens larger than phones, it has the sidebar which has a contrasting heading and a single column of partners. On small screens, the Startups section becomes just another grid at the bottom of the page (phone screens generally hold 2 partner info cards on each row).