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

Display more profiles on the profile drop-down list, v2 (fixed for low res, fixed linter) #4359

Merged
merged 7 commits into from
Nov 22, 2023
Merged

Display more profiles on the profile drop-down list, v2 (fixed for low res, fixed linter) #4359

merged 7 commits into from
Nov 22, 2023

Conversation

CelularBat
Copy link
Contributor

@CelularBat CelularBat commented Nov 20, 2023 •
edited
Loading

Fixed previous PR: #4357

Current profile list has hardcoded size, so it can always display only 5 profiles. If you have more profiles you have to scroll, if you have much more profiles, like 15+ scrolling for what you want to find in that little 5-items box is inconvenient.

This extends profile list size to fit the amount of profiles.

  • it scales with window size and resolution.
  • for resolution 800x600 and any lower it's exactly the same as it was before

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #2871

Description

it changes 4 lines of .css

Screenshots

before:
p1
after (at 1920x1080)
p2
after (at 1920x1080 ,random window size)
p3
after (at 1280x720)
p4

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

@CelularBat
Update ft-profile-selector.css
919910d 
Fixed for low res, fixed linter complains
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 20, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 20, 2023 07:59
PikachuEXE
PikachuEXE previously approved these changes Nov 20, 2023
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.

Height is auto adjusted until at least 5 profiles are shown (like now)
Works great~

kommunarr
kommunarr reviewed Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

Great job @CelularBat! I do have two very minor nitpicks if that's okay:

  1. nitpick: Dropdown clips into the bottom bar on mobile, so you might need a media query to handle that. Totally fine to pass on if this one is too much trouble to resolve.
    Screenshot_20231120_075952

  2. suggestion (non-blocking): A code comment explaining the max-block-size will help if we ever decide to resize the bars at some point, or something like that.

auto-merge was automatically disabled November 22, 2023 02:50

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 02:50
auto-merge was automatically disabled November 22, 2023 02:50

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 02:51
@CelularBat
Update ft-profile-selector.css
75af05d 
adjust for screen with horizontal navbar (mobile)
auto-merge was automatically disabled November 22, 2023 04:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 04:28
@CelularBat
Copy link
Contributor Author

CelularBat commented Nov 22, 2023 •
edited
Loading

@jasonhenriquez

Great job @CelularBat! I do have two very minor nitpicks if that's okay:

1. **nitpick:** Dropdown clips into the bottom bar on mobile, so you might need a [media query](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Using_media_queries) to handle that. Totally fine to pass on if this one is too much trouble to resolve.
   ![Screenshot_20231120_075952](https://user-images.githubusercontent.com/84899178/284288584-1984ca5c-18e8-43b9-a4ef-6d9bc0c8199d.png)

2. **suggestion (non-blocking):** A code comment explaining the `max-block-size` will help if we ever decide to resize the bars at some point, or something like that.
  1. Done. But some testing on mobile would be nice, because I've tested it only on desktop.
  2. Comment added

PikachuEXE
PikachuEXE requested changes Nov 22, 2023
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.

Small width seems fine
image
image

Only minor code style issues remain

src/renderer/components/ft-profile-selector/ft-profile-selector.css Outdated Show resolved Hide resolved
src/renderer/components/ft-profile-selector/ft-profile-selector.css Outdated Show resolved Hide resolved
src/renderer/components/ft-profile-selector/ft-profile-selector.css Outdated Show resolved Hide resolved
@CelularBat @PikachuEXE
Update src/renderer/components/ft-profile-selector/ft-profile-selecto…
49b2213 
…r.css

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
auto-merge was automatically disabled November 22, 2023 05:58

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 05:58
@CelularBat @PikachuEXE
Update src/renderer/components/ft-profile-selector/ft-profile-selecto…
7b23737 
…r.css

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
auto-merge was automatically disabled November 22, 2023 05:58

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 05:58
@CelularBat @PikachuEXE
Update src/renderer/components/ft-profile-selector/ft-profile-selecto…
33b1bed 
…r.css

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
auto-merge was automatically disabled November 22, 2023 05:59

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 05:59
CelularBat
CelularBat commented Nov 22, 2023
View reviewed changes
Copy link
Contributor Author

@CelularBat CelularBat left a comment

Choose a reason for hiding this comment

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

ok

kommunarr
kommunarr approved these changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

LGTM. Tested with four main cases:

  1. Few profiles desktop view
  2. Few profiles tablet & mobile view
  3. Many profiles desktop view
  4. Many profiles tablet & mobile view

Thanks for being so patient with our nitpicks! Contributing the first time to a new repo can be scary, but you've been very responsive and cooperative.

1. Done. But some testing on mobile would be nice, because I've tested it only on desktop.

Just so you know for the future, you can pretty accurately emulate the mobile experience using Device Mode in Chrome DevTools.

@FreeTubeBot FreeTubeBot merged commit 62e7b43 into FreeTubeApp:development Nov 22, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 22, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Nov 23, 2023
@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05
6ea2b7a 
* development:
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Kurdish (Central))
  Translated using Weblate (Spanish)
  Translated using Weblate (Kurdish (Central))
  Translated using Weblate (Kurdish)
  A new way to subscribe (FreeTubeApp#4238)
  Display more profiles on the profile drop-down list, v2 (fixed for low res, fixed linter)  (FreeTubeApp#4359)
  Translated using Weblate (Estonian)
  Translated using Weblate (Serbian)
  Translated using Weblate (Arabic)
  Translated using Weblate (Chinese (Simplified))
  Display currently watching viewer count on live streams (FreeTubeApp#4206)
  Translated using Weblate (Spanish)
  Translated using Weblate (Czech)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Italian)
  Fix block channel channel ID validation (FreeTubeApp#4366)
@CelularBat CelularBat deleted the PR-profileList_fixed branch November 26, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@kommunarr kommunarr kommunarr approved these changes

@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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@CelularBat @PikachuEXE @ChunkyProgrammer @kommunarr @FreeTubeBot