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

Large screen style #1584

Closed
wants to merge 23 commits into from
Closed

Large screen style #1584

wants to merge 23 commits into from

Conversation

SleeplessOne1917
Copy link
Member

Closes #1536. Here is what the pages look like now:

Home

image

Community Page

image

Communities

image

Create Post

image

Create Community

image

Search

image

Admin Settings

image

Inbox

image

Reports

image

Modlog

image

Instances

image

I still have to do settings and profile. Will update the PR with screenshots when done.

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

Hm it seems like some of these changes are unrelated to screen widening?

@SleeplessOne1917
Copy link
Member Author

I also removed a bit of unnecessary markup.

Comment on lines +61 to +67
<div className="col-12 col-lg-6 offset-lg-3 mb-4">
<h5>{I18NextService.i18n.t("verify_email")}</h5>
{this.state.verifyRes.state === "loading" && (
<h5>
<Spinner large />
</h5>
)}
Copy link
Member Author

Choose a reason for hiding this comment

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

All this page displays is a title and a loading spinner. @dessalines should we still keep this file?

src/assets/css/main.css Outdated Show resolved Hide resolved
src/assets/css/main.css Outdated Show resolved Hide resolved
src/shared/components/comment/comment-nodes.tsx Outdated Show resolved Hide resolved
@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

Post Listing images are now "portrait"-oriented, and are no longer top-aligned with the post title:
Screenshot 2023-06-25 at 1 11 43 PM

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

Kind of throwing a wrench into things here, but I'm not sure this is even desirable in the first place? I don't know that this makes good use of the extra space gained; it will mostly just make things further apart.

I could see changing the max-width to something else (in fact it's larger in Bootstrap 5, and I overwrote the value of $max-width when switching to BS5 to maintain the existing look), but indefinite width feels weird to me.

Screenshot 2023-06-25 at 1 14 21 PM

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

From the original issue:

over half of my browser window on the front page is empty space.

There's still empty space; it's now just between the main and the sidebar.

I think this is this one user's personal preference and shouldn't necessarily be globally adopted.

@alectrocute
Copy link
Contributor

From the original issue:

over half of my browser window on the front page is empty space.

There's still empty space; it's now just between the main and the sidebar.

I think this is this one user's personal preference and shouldn't necessarily be globally adopted.

I agree completely.

@SleeplessOne1917
Copy link
Member Author

Any thoughts on how to make it better? I think there's still too much whitespace but also an improvement on what's there currently.

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

  1. Increase the max-width of the whole site
  2. Change the sidebar to be 3 columns, not 4, on wide

@SleeplessOne1917
Copy link
Member Author

Post Listing images are now "portrait"-oriented, and are no longer top-aligned with the post title:

I thought the top aligned listing images looked funny on post listings with more vertical space. I can change it back if you'd prefer.

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

Here's an example of another way we might address this: #1590

} = this.state;

const hasCommunities =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh. I'll put it back.

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

It seems like there's a lot going on here that, while good improvements, don't really have to do with making things wider. Can these be moved to a separate chore PR?

@SleeplessOne1917
Copy link
Member Author

What specifically do you think doesn't belong in this PR? I've gotten rid of everything not related to displaying on larger screen sizes at this point.

@jsit
Copy link
Contributor

jsit commented Jun 25, 2023

Some of this looks a little wonky to me? This sidebar looks too small, and the comments aren't left-aligned with the post content

Screenshot 2023-06-25 at 3 58 30 PM

I'd kind of like to see #1590 merged in first, then some more fine-tuning done if necessary.

I also like how this PR makes some semantic fixes, but they make the diffs harder to read than they need to be for such a large PR.

It might be easier if this were broken up into smaller PRs for each page.

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.

Add CSS styles for min-width beyond 1200px
3 participants