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

Update icon for channels button on side nav #5273

Merged
merged 5 commits into from
Jul 25, 2024
Merged

Update icon for channels button on side nav #5273

merged 5 commits into from
Jul 25, 2024

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Jun 17, 2024 •
edited
Loading

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

#5003 (comment)

Description

Update icon for channels button so an icon similar to the existing one can be used for remote playlist in #5003

Screenshots

Width reduced from 1.25em to 1em:
image
image
height reduced from 1.3em > 1.15em
image

Mobile
image

Testing

  • Check the new icon in different modes

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 17, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 17, 2024 05:35
@kommunarr
Copy link
Collaborator

suggestion (non-blocking): The alignment of the icons is now bugging me given that this new icon is pretty wide. Could we left-align the icons?

@PikachuEXE
Copy link
Collaborator Author

image
image

I understand the content is wider than others but as it's already fixed width I doubt that left-align (I don't even know what it means) fixes anything

@PikachuEXE
Copy link
Collaborator Author

Or just use icon without large asymmetry

e.g. user
image

@kommunarr
Copy link
Collaborator

left-align (I don't even know what it means)

Screenshot_20240617_072734

Here's how it looks like with a one-line change (the one highlighted) for reference. Needs a bit more tweaking for the other two cases, but just to demonstrate what I mean

@PikachuEXE
Copy link
Collaborator Author

In that case I might as well post all screenshots for using user for easier comparison
image
image
image

@kommunarr
Copy link
Collaborator

I'd prefer user-check because it's more clearly conveying "subscribing" with the checkmark. list and user seem roughly equivalent in terms of how effectively they describe the meaning. I will leave it at that and go along with whichever decision we make for this.

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Jun 18, 2024 •
edited
Loading

I am not quite sure that I agree with the left align for icons (for text I can understand)
Due to varying gap especially for Trending & Playlists
Centered
image
Left align
image
Right align
image

Youtube got no such issue
image

Still finding better solutions

Update 1: Add Right align screenshot
Update 2: Fix Right align screenshot

@kommunarr
Copy link
Collaborator

YouTube doesn't seem to have an issue because the icons are all very similar in size. We could try scaling this particular icon to a small width (with height auto)

@PikachuEXE
Copy link
Collaborator Author

Width reduced from 1.25em to 1em:
image
image
image

@kommunarr
Copy link
Collaborator

kommunarr commented Jun 18, 2024 •
edited
Loading

Ooh! We're very close now. That looks good on everything except the Hide Labels view, where the torso of the icon is poking out. Maybe we need some individual & commented patch for this specific Hide Labels case & icon, like a forced left padding that aligns the user part of the icon with the center.

@PikachuEXE
Copy link
Collaborator Author

height reduced from 1.3em > 1.15em
image

kommunarr
kommunarr previously approved these changes Jun 18, 2024
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! Please update the OP with the newest screenshots when you have a sec!

kommunarr
kommunarr reviewed Jun 18, 2024
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.

Please update with the suggested change

@PikachuEXE
Copy link
Collaborator Author

OP screenshots updated, code change pushed

@jasonhenriquez I meant to push code changes after you feel fine with the preview XD

kommunarr
kommunarr previously approved these changes Jun 18, 2024
View reviewed changes
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

I do agree that this icon is way better than the one we had before but im after all those changes im still a bit bothered by the alignment. If this is the best that can be done than i just have to deal with it but i just wanted to point out that it still looks a bit off to me

@kommunarr
Copy link
Collaborator

I think you can make an argument for making the bounding box of the icon centered around the user torso, and having the checkmark part not included in the positioning check. But I don't know of a good way to do that at the moment.

@PikachuEXE
Copy link
Collaborator Author

Manually adjusted the horizontal position
If it looks off please test locally to find the value that look right for you and share :)
image
image
image

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc requested changes Jul 13, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

Icon in mobile view needs to be adjusted too

Capture

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

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@PikachuEXE
Merge branch 'development' into feature/channel-page-update-icon
6938d16 
* development: (78 commits)
  Bump the stylelint group with 2 updates (FreeTubeApp#5411)
  Bump rimraf from 5.0.8 to 6.0.1 (FreeTubeApp#5412)
  Translated using Weblate (Indonesian)
  Bump the babel group across 1 directory with 3 updates (FreeTubeApp#5419)
  Bump sass from 1.77.6 to 1.77.8 (FreeTubeApp#5413)
  Bump webpack from 5.92.1 to 5.93.0 (FreeTubeApp#5414)
  Bump lefthook from 1.7.1 to 1.7.2 (FreeTubeApp#5415)
  Bump electron from 31.1.0 to 31.2.0 (FreeTubeApp#5416)
  Fix about page not reacting to locale changes (FreeTubeApp#5393)
  Delete .github/workflows/report.yml (FreeTubeApp#5395)
  Bump version number to v0.21.1
  Fix Invidious DASH manifest generation (FreeTubeApp#5387)
  ^ Update YouTube.js to 10.1.0 (FreeTubeApp#5384)
  Fix Invidious API error toasts saying undefined (FreeTubeApp#5380)
  Bump lefthook from 1.6.18 to 1.7.1 (FreeTubeApp#5376)
  Bump eslint-plugin-vue from 9.26.0 to 9.27.0 in the eslint group (FreeTubeApp#5371)
  Bump rimraf from 5.0.7 to 5.0.8 (FreeTubeApp#5373)
  Bump npm-run-all2 from 6.2.0 to 6.2.2 (FreeTubeApp#5374)
  Bump marked from 13.0.1 to 13.0.2 (FreeTubeApp#5375)
  Translated using Weblate (Portuguese (Brazil))
  ...

# Conflicts:
#	src/renderer/components/side-nav/side-nav.css
@PikachuEXE
* Update mobile menu item icon
7a130ac
@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@PikachuEXE
Copy link
Collaborator Author

PikachuEXE commented Jul 19, 2024 •
edited
Loading

@efb4f5ff-1298-471a-8973-3d47447115dc
Updated now, did not see your message (probably deleted accidentally)
image

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jul 19, 2024
@PikachuEXE PikachuEXE mentioned this pull request Jul 21, 2024
Draft
1 task
@FreeTubeBot FreeTubeBot merged commit 0ac277a into FreeTubeApp:development Jul 25, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 25, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jul 25, 2024
@PikachuEXE
Merge branch 'development' into custom-builds/current
76b6e1f 
* development:
  Bump version number to v0.21.2
  Update YouTube.js to 10.2.0 (FreeTubeApp#5465)
  Update icon for channels button on side nav (FreeTubeApp#5273)
  Translated using Weblate (Azerbaijani)
  Fix getRegions script and update the regions (FreeTubeApp#5450)
  More flexible hardware acceleration for Linux (FreeTubeApp#5438)
  Bump the eslint group with 3 updates (FreeTubeApp#5441)
  Bump lefthook from 1.7.2 to 1.7.5 (FreeTubeApp#5445)
  Bump the fortawesome group with 4 updates (FreeTubeApp#5442)
  Bump swiper from 11.1.4 to 11.1.5 (FreeTubeApp#5443)
  Bump electron from 31.2.0 to 31.2.1 (FreeTubeApp#5444)
  Fix scss deprecation warnings (FreeTubeApp#5429)
  Bump the stylelint group with 2 updates (FreeTubeApp#5411)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@absidue absidue absidue approved these changes

@kommunarr kommunarr Awaiting requested review from kommunarr

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

Successfully merging this pull request may close these issues.
6 participants
@PikachuEXE @kommunarr @efb4f5ff-1298-471a-8973-3d47447115dc @absidue @ChunkyProgrammer @FreeTubeBot