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 About page to display correct Freetube logo based on currently set theme #5126

Merged
merged 5 commits into from
Jun 18, 2024
Merged

Update About page to display correct Freetube logo based on currently set theme #5126

merged 5 commits into from
Jun 18, 2024

Conversation

ducks
Copy link
Contributor

@ducks ducks commented May 19, 2024

Update About page to display correct Freetube logo based on currently set theme

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #4020

Description

This PR updates the About page view to display the Freetube logo similarly to the top nav. It adds two HTML elements in place of the inline svg. Then we can use CSS to style those elements with the logo variables set by each theme.

Screenshots

before:
before-nordic
before-pastel-pink

after:
after-nordic
after-pastel-pink

Desktop

  • OS: linux
  • OS Version: manjaro 24
  • FreeTube version: v0.20.0 beta

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 19, 2024 07:15
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 19, 2024
absidue
absidue reviewed May 19, 2024
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 for you pull request! I haven't had a chance to test it yet, so for the moment this is just a code review with a small code cleanup suggestion.

src/renderer/views/About/About.scss Outdated Show resolved Hide resolved
auto-merge was automatically disabled May 19, 2024 15:59

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 19, 2024 15:59
@absidue
Copy link
Member

absidue commented May 19, 2024

Could you please use relative sizes instead of absolute pixel amounts, as the logo is larger than it was before on smaller screen sizes and on mobile is gigantic and split onto two separate lines. I've provided some before and after screenshots below demonstrating the issue:

Desktop before

before

Desktop with this pull request

after

Mobile before

before-mobile

Mobile with this pull request

after-mobile

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 19, 2024
@ducks
Copy link
Contributor Author

ducks commented May 19, 2024

sure, np. I started with rems locally but saw some px values and decided to just stay consistent to start.

I added some css to specifically break the Freetube text to a newline when it gets smaller but I can remove that. The top nav just hides the logo text and shows only the icon below 680px. should I replicate that here?

@absidue
Copy link
Member

absidue commented May 19, 2024

The top nav just hides the logo text and shows only the icon below 680px. should I replicate that here?

We have more space on the About page, so ideally it should like the same as it was before, just in different colors.

@ducks
Copy link
Contributor Author

ducks commented May 19, 2024

okie doke. I will work on making it match what was there size and layout wise. I mostly ask because before, the about page just had a single inline svg that scaled down but this PR adds the two elements that get styled so it could be more flexible if desired.

@github-actions GitHub Actions
Copy link
Contributor

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

auto-merge was automatically disabled June 8, 2024 23:03

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 8, 2024 23:03
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 8, 2024

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

@ducks
Copy link
Contributor Author

ducks commented Jun 8, 2024

I ended up pivoting on how to implement this and added an ft-logo-full component. this way, the logo can be imported directly as an svg and then styled with css using fill. I added classes to the svg and then a few new logo variables to each theme that will style the svg when enabled.

I named it ft-logo-full because this could be done for the separate icon and text parts in the top nav and could eliminate the need for logo files for each theme.

mobile before and after:

ft-before-mobile

ft-after-mobile

tablet before and after:

ft-before-tablet

ft-after-tablet

desktop before and after:

ft-before-desktop

ft-after-desktop

auto-merge was automatically disabled June 8, 2024 23:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 8, 2024 23:23
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc reviewed Jun 10, 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.

LGTM, will approve after @jasonhenriquez and @absidue

auto-merge was automatically disabled June 10, 2024 19:03

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 10, 2024 19:03
kommunarr
kommunarr reviewed Jun 10, 2024
View reviewed changes
src/renderer/themes.css Outdated Show resolved Hide resolved
auto-merge was automatically disabled June 12, 2024 19:52

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) June 12, 2024 19:53
kommunarr
kommunarr approved these changes Jun 13, 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.

Works as intended. Not sure if there's a way to do this while keeping the SVG as a file using attribute selectors, but I haven't been able to find one.

@ducks
Copy link
Contributor Author

ducks commented Jun 13, 2024 •
edited
Loading

Works as intended. Not sure if there's a way to do this while keeping the SVG as a file using attribute selectors, but I haven't been able to find one.

@jasonhenriquez do you mean doing this without a component and just the svg file?

@kommunarr
Copy link
Collaborator

Or just a reference to the file rather than having it in a template. I don't think so, from what I've seen, so what's present now should be fine.

@ducks
Copy link
Contributor Author

ducks commented Jun 13, 2024

Or just a reference to the file rather than having it in a template. I don't think so, from what I've seen, so what's present now should be fine.

ah yeah. It is possible to import the svg in the js and include it as a component that way, but that would require vue-svg-loader or similar.

@kommunarr kommunarr added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels Jun 15, 2024
@FreeTubeBot FreeTubeBot merged commit d42e2ad into FreeTubeApp:development Jun 18, 2024
6 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jun 18, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jun 20, 2024
@PikachuEXE
Merge branch 'development' into feature/history/remember-search-query
3c71c71 
* development: (120 commits)
  Update version number to v0.21.0
  Remove limited donation methods (FreeTubeApp#5290)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese (Portugal))
  Avoid cloning all profiles when subscribing and unsubscribing (FreeTubeApp#5289)
  Fix arrow keys not working in the Create New Playlist prompt (FreeTubeApp#5243)
  Bump ws from 8.16.0 to 8.17.1 (FreeTubeApp#5291)
  Remove a few bits of unused code (FreeTubeApp#5287)
  Update About page to display correct Freetube logo based on currently set theme (FreeTubeApp#5126)
  Translated using Weblate (Croatian)
  * Update playlist page titles (FreeTubeApp#5271)
  Update Invidious instances list (FreeTubeApp#5288)
  Translated using Weblate (Polish)
  Bump the eslint group with 2 updates (FreeTubeApp#5275)
  Bump marked from 12.0.2 to 13.0.0 (FreeTubeApp#5276)
  Bump sass from 1.77.4 to 1.77.5 (FreeTubeApp#5277)
  Bump lefthook from 1.6.15 to 1.6.16 (FreeTubeApp#5279)
  Bump webpack from 5.91.0 to 5.92.0 (FreeTubeApp#5278)
  Update Flatpak PR Workflow to work with updated module (FreeTubeApp#5270)
  Translated using Weblate (Portuguese)
  ...
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jun 20, 2024
@PikachuEXE
Merge branch 'development' into feature/subscription-cache
5a08fd0 
* development: (102 commits)
  Update version number to v0.21.0
  Remove limited donation methods (FreeTubeApp#5290)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese (Portugal))
  Avoid cloning all profiles when subscribing and unsubscribing (FreeTubeApp#5289)
  Fix arrow keys not working in the Create New Playlist prompt (FreeTubeApp#5243)
  Bump ws from 8.16.0 to 8.17.1 (FreeTubeApp#5291)
  Remove a few bits of unused code (FreeTubeApp#5287)
  Update About page to display correct Freetube logo based on currently set theme (FreeTubeApp#5126)
  Translated using Weblate (Croatian)
  * Update playlist page titles (FreeTubeApp#5271)
  Update Invidious instances list (FreeTubeApp#5288)
  Translated using Weblate (Polish)
  Bump the eslint group with 2 updates (FreeTubeApp#5275)
  Bump marked from 12.0.2 to 13.0.0 (FreeTubeApp#5276)
  Bump sass from 1.77.4 to 1.77.5 (FreeTubeApp#5277)
  Bump lefthook from 1.6.15 to 1.6.16 (FreeTubeApp#5279)
  Bump webpack from 5.91.0 to 5.92.0 (FreeTubeApp#5278)
  Update Flatpak PR Workflow to work with updated module (FreeTubeApp#5270)
  Translated using Weblate (Portuguese)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommunarr kommunarr kommunarr 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

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

Successfully merging this pull request may close these issues.

[Feature Request]: Change icon color of FT logo on About page based on set theme
5 participants
@ducks @absidue @kommunarr @efb4f5ff-1298-471a-8973-3d47447115dc @FreeTubeBot