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

Maintain 12 columns per row in the layout #4065

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

Conversation

jcoyne
Copy link
Contributor

@jcoyne jcoyne commented Mar 1, 2024

This fixes the problem where the subheader was not aligning with the toolbar.

The previous margin was added #654 to keep the leftside mini-toolbar from crashing into the panels. But we can accomodate for this by setting the right-margin of the rows in the content.

Before:
Screenshot 2024-03-01 at 2 19 05 PM

After:
Screenshot 2024-03-01 at 2 19 16 PM

This also prevents these bugs, where the toolbar or browsing can get stuck under the mini-menu.
Screenshot 2024-03-01 at 2 56 23 PM

Screenshot 2024-03-01 at 2 59 44 PM

Approved by Darcy https://stanfordlib.slack.com/archives/G018TJ20XGE/p1709655061981309?thread_ts=1709654314.383919&cid=G018TJ20XGE

This fixes the problem where the subheader was not aligning with the toolbar.
Copy link
Contributor

@dnoneill dnoneill left a comment

Choose a reason for hiding this comment

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

Around 952 px I think the margins look a little weird. I think we need some clean up for larger screens to match what is on prod.

On this branch
Screenshot 2024-03-11 at 10 48 04 AM

On prod
Screenshot 2024-03-11 at 10 48 11 AM

@jcoyne
Copy link
Contributor Author

jcoyne commented Mar 11, 2024

@dnoneill I don't understand what you are asking me to do.

@dnoneill
Copy link
Contributor

@jcoyne If you look at this branch the margins for the record view on a screen 952px don't look even. I think at a larger size there probably isn't a need for the margin-right css variable. I think we are going to want to mirror what is on production for some of the larger screen sizes. From around 822px to around 970px margins look a little odd and off balance (right much larger than left).

@jcoyne
Copy link
Contributor Author

jcoyne commented Mar 11, 2024

margins for the record view on a screen 952px don't look even

I think that's the change that was necessary so that the mini-menu doesn't collide with the stuff in the middle. I'm not sure how to have it both ways.

@dnoneill
Copy link
Contributor

dnoneill commented Mar 11, 2024

What if the margin is changed to margin: 0px calc(-30px * .5 + 35px); So that you have equal margins on either side?

I am including some phone screenshots to compare.

This branch:
Screenshot 2024-03-11 at 3 09 15 PM

With margin set to margin: 0px calc(-30px * .5 + 35px);
Screenshot 2024-03-11 at 3 10 02 PM

With margin set to margin: 0px calc(-30px * .5 + 35px); and .content padding set to 0
Screenshot 2024-03-11 at 3 10 20 PM

@jcoyne
Copy link
Contributor Author

jcoyne commented Mar 11, 2024

@dnoneill that feels like we're not taking advantage of all the available real-estate, making the design look more cramped than necessary.

@dnoneill
Copy link
Contributor

@dbranchini Do you have an opinion on this?

@dbranchini
Copy link

dbranchini commented Mar 11, 2024

Out of all of the screenshots, I like the third example best, but I also see Justin's point and wonder if the left and right margins could be smaller. I suppose they can't because you're providing room for the side toolbar. In an ideal world, I'd prefer that the side toolbar overlap the content or (even better) was collapsible into a single button (like Material's floating action button). When I look at it on my phone in production, it does seem to overlap some content (not consistently), but there also seems to be a horizontal scroll too? The up/down button on the search results page also seems to overlap content.

@dnoneill dnoneill dismissed their stale review June 21, 2024 20:14

don't remember blockers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants