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.

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

Fix blog banner layout #2300

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Fix blog banner layout #2300

merged 3 commits into from
Jul 7, 2022

Conversation

vallode
Copy link
Contributor

@vallode vallode commented Jun 8, 2022 •
edited
Loading

Title

Pull Request Type
Please select what type of pull request this is:

  • Bugfix

Related issue
Closes #2297

Description
Fixes a layout error with the blog notification banner

Screenshots (if appropriate)
Before:
image

After:
image

Desktop (please complete the following information):

  • OS: [e.g. iOS] 10
  • OS Version: [e.g. 22] Debian
  • FreeTube version: [e.g. 0.8] upstream/development

Additional context
Cheers chonky for pointing it out!

@PrestonN PrestonN enabled auto-merge (squash) June 8, 2022 11:58
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 8, 2022
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jun 19, 2022 •
edited
Loading

could u also create a PR in the RC branch?

edit: @vallode is there a way to force the blog post banner so i can test it?

PikachuEXE
PikachuEXE previously approved these changes Jul 4, 2022
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.

Local test result:

Before fix
image

After fix
image

@absidue
Copy link
Member

absidue commented Jul 4, 2022

This PR unfortunately doesn't fix the issue entirely on my end.

New version banner
Testing:

  1. change version in package.json to 0.15.0
  2. yarn run dev

Result:
new_version_banner

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

Result:
new_blog_banner

absidue
absidue requested changes Jul 4, 2022
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Looks like the banner is greedy and tries to take up as much space as possible.

Loading screen while searching:
loading_screen

Search results:
search_results

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 4, 2022
@PikachuEXE
Copy link
Collaborator

@vallode
I tried to fix it and here is my maybe WIP result
development...PikachuEXE:FreeTube:fix-blog-notification-pika

Screen.Recording.2022-07-05.at.13.37.57.mov

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Jul 5, 2022 •
edited
Loading

You can experiment with more properties with
Flexy Boxes(already got my updates applied)

auto-merge was automatically disabled July 5, 2022 10:56

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 5, 2022 10:57
@vallode
Copy link
Contributor Author

vallode commented Jul 5, 2022

Thanks everyone for the reminders, been going through some personal stuff. Fixed it for now, should I open a new PR for the release candidate too?

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

should I open a new PR for the release candidate too?

@vallode yes please! :)

PikachuEXE
PikachuEXE approved these changes Jul 6, 2022
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.

Tested locally

absidue
absidue approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Thank you, looks good now

@absidue absidue added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jul 6, 2022
GilgusMaximus
GilgusMaximus approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@GilgusMaximus GilgusMaximus left a comment

Choose a reason for hiding this comment

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

Tested locally, works well. No problems at different sizes or with different amount of videos

@PrestonN PrestonN merged commit 2340af0 into FreeTubeApp:development Jul 7, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 7, 2022
GilgusMaximus pushed a commit to GilgusMaximus/FreeTube that referenced this pull request Jul 7, 2022
@vallode @GilgusMaximus
Fix blog banner layout (FreeTubeApp#2300)
2a128f4 
* Fix blog banner layout

* Fix banner stretching with no content
@GilgusMaximus GilgusMaximus mentioned this pull request Jul 7, 2022
Merged
1 task
PrestonN pushed a commit that referenced this pull request Jul 7, 2022
@GilgusMaximus @vallode
Fix blog banner layout (#2300) (#2383)
04c276c 
* Fix blog banner layout

* Fix banner stretching with no content

Co-authored-by: vallode <[email protected]>
@vallode vallode deleted the fix-blog-notification branch July 7, 2022 19:44
@vallode vallode restored the fix-blog-notification branch July 7, 2022 19:44
@vallode vallode deleted the fix-blog-notification branch July 7, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer left review comments

@absidue absidue absidue approved these changes

@GilgusMaximus GilgusMaximus GilgusMaximus approved these changes

@PikachuEXE PikachuEXE PikachuEXE approved these changes

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: New Blog Notification is too large
7 participants
@vallode @efb4f5ff-1298-471a-8973-3d47447115dc @absidue @PikachuEXE @GilgusMaximus @ChunkyProgrammer @PrestonN