Skip to content

Normalize margin at top of page #7040

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: development
Choose a base branch
from
Open

Normalize margin at top of page #7040

wants to merge 4 commits into from

Conversation

MarmadileManteater
Copy link
Contributor

Normalize margin at top of page

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other (visual consistency)

Description

The margin above the top of each view is not consistent. Sometimes, its narrow. Other times, it is wide. This PR aims to improve consistency so that the margin at the top is always the same.

Screenshots

before
2025-03-19-204247_hyprshot
after
2025-03-19-204211_hyprshot
before
2025-03-19-203631_hyprshot
after
2025-03-19-203518_hyprshot

Testing

Compare all the different variants of the top margin

  1. Look at a view with a floating refresh widget
  2. Look at a view without a floating refresh widget
  3. Reduce window width to below 680
  4. Look at a view with a floating refresh widget
  5. Look at a view without a floating refresh widget
  6. Close the banner
  7. Look at a view with a floating refresh widget
  8. Look at a view without a floating refresh widget

Desktop

  • OS: Fedora Linux w/ hyprland
  • OS Version: 41 (Workstation Edition) x86_64
  • FreeTube version: 5403fec

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) March 20, 2025 00:56
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 20, 2025
PikachuEXE
PikachuEXE reviewed Mar 20, 2025
View reviewed changes
src/renderer/App.css Outdated
margin-inline: auto;
margin-block-start: -2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is banner used outside of ftNotificationBanner?
If not ftNotificationBanner itself got margin already
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. Nice catch.

PikachuEXE
PikachuEXE previously approved these changes Mar 20, 2025
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

single banner tested

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Tested with 2 banners and with this PR it looks a bit weird

Before:

VirtualBoxVM_HQmCcSvY1p.mp4

After:

VirtualBoxVM_nstpHHNVqa.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Mar 24, 2025 •
edited
Loading

Ugh cant get the blog banner to show up. Anyone know how to force the banner to show?

Edit: nvm found the answer #2300 (comment)

New blog banner
Testing:

  1. yarn run dev
  2. In the devtools: Application -> Local Storage -> http://localhost:9080
  3. Delete lastAppWasRunning key
  4. Close FreeTube
  5. yarn run dev

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Mar 24, 2025 •
edited
Loading

I do think the spacing between the banners is still a bit much compared to the non mobile view. Also the spacing between the last banner and the space on the bottom seems a bit much

VirtualBoxVM_a0AgcQPh8P.mp4

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 8, 2025

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: stale PR: waiting for review For PRs that are complete, tested, and ready for review labels Apr 17, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 5, 2025

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE left review comments

@absidue absidue Awaiting requested review from absidue

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc Awaiting requested review from efb4f5ff-1298-471a-8973-3d47447115dc

@kommunarr kommunarr Awaiting requested review from kommunarr

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
PR: changes requested PR: WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarmadileManteater @efb4f5ff-1298-471a-8973-3d47447115dc @PikachuEXE