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.

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

Change Video not available PNG image to a vector drawable #5925

Closed
wants to merge 19 commits into from

Conversation

sherlockbeard
Copy link
Contributor

@sherlockbeard sherlockbeard commented Mar 27, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Fixes #5609

Fixes the following issue(s)

removed png image and cropped and changed into vector drawable.

photo6269502245057964727

@AudricV AudricV changed the title Isa4563 Change Video not available PNG image to a vector drawable Mar 27, 2021
@AudricV AudricV added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Mar 27, 2021
@Stypox
Copy link
Member

Stypox commented Mar 27, 2021

Uh, interesting :-)
Remove the ic_ from the file name, ic_ stands for "icon" but this is not an icon ;-)

@TobiGr TobiGr added the localisation / translation Everything that has to do with translations or Weblate label Mar 27, 2021
@Stypox
Copy link
Member

Stypox commented Mar 28, 2021

I tried this, but the image seems to me a little bit uncentered. Could you improve the vector drawable by making sure the two points representing the nose lie at the center?

@sherlockbeard
Copy link
Contributor Author

which one is better ?
1)
photo6271376581670841470

photo6271376581670841471

@Stypox
Copy link
Member

Stypox commented Mar 28, 2021

I prefer the first, but maybe just put it a little bit to the right and add some padding (like before)

@sherlockbeard
Copy link
Contributor Author

current

photo6271376581670841477

Stypox
Stypox previously approved these changes Mar 28, 2021
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

@sherlockbeard that looks good, thank you for the changes :-D
I'm gonna keep this open for some days, and if nobody complains I'm gonna merge it @TobiGr @TiA4f8R @mhmdanas @opusforlife2

@AudricV
Copy link
Member

AudricV commented Mar 28, 2021

Just a thing that I want to know is your advice about the background color of the monkey. Should this be black or grey like on the PNG image before?

@Stypox
Copy link
Member

Stypox commented Mar 29, 2021

@TiA4f8R I think this is ok. One could argue that it should be inverted for white theme, though, what do you think?

@opusforlife2
Copy link
Collaborator

How about black (like in the preview images) for black theme, dark grey for dark theme, and light grey for light theme?

@AudricV
Copy link
Member

AudricV commented Mar 29, 2021

Nice idea!

@Stypox
Copy link
Member

Stypox commented Mar 31, 2021

@sherlockbeard please create three different drawables with the same shape but with different colors:

  • white background & black foreground for white theme
  • gray background & white foreground for dark theme
  • black background & white foreground for black theme

And then select the correct drawable according to the current theme.

@triallax
Copy link
Contributor

@sherlockbeard where did you get the vector drawable from? Did you make it yourself?

@sherlockbeard
Copy link
Contributor Author

@mhmdanas Converted the image online. Then positioned it by changing the value's in Editor.

@alienthief
Copy link

alienthief commented Apr 1, 2021

@sherlockbeard thank you for this. Could you zoom out the sad face a bit and move the bellow text up in the video window? It would be more convenient as I mentioned in my issue (the one this PR fixes)

@Stypox
Copy link
Member

Stypox commented Apr 1, 2021

@alienthief I don't think the image should be further zoomed out, and moving the error panel higher is not related to this PR

@Stypox
Copy link
Member

Stypox commented Apr 3, 2021

Maybe in the XML you can set the background color of detailThumbnailImageView to the same color as important text (e.g. check the color of the video title and copy that)

@TobiGr
Copy link
Contributor

TobiGr commented Apr 4, 2021

Thank you. That's what I get on a TV / tablet

Screenshots

@TacoTheDank
Copy link
Member

@mhmdanas Converted the image online. Then positioned it by changing the value's in Editor.

Did you use some sort of online SVG path editor, or is there an actual site that can convert images to vectors? I'm interested to know what site you used to do this 🤔

@sherlockbeard
Copy link
Contributor Author

@TacoTheDank
https://shapeshifter.design/
https://vectr.com

@TobiGr
Copy link
Contributor

TobiGr commented May 4, 2021

@sherlockbeard CAn you please address #5925 (comment)

@Stypox
Copy link
Member

Stypox commented Jun 8, 2021

@sherlockbeard ping

@sherlockbeard
Copy link
Contributor Author

@TobiGr @Stypox . No i don't think i can mention the #5925 . i tried it only in mobile before the comment .After testing it on tablet i got the same result as in #5925 . I tried to search for a solution but i was not able to come up with a answer . Sorry i am noob developer .

@nadiration
Copy link
Contributor

@sherlockbeard Did you abandon thid? This is kinda important. We've been waiting for this.

@sherlockbeard
Copy link
Contributor Author

sherlockbeard commented Aug 4, 2021

@nadiration . Sorry i have to abandon this pull request . i will be closing this pull request

@nadiration
Copy link
Contributor

nadiration commented Aug 12, 2021

@nadiration . Sorry i have to abandon this pull request . i will be closing this pull request

@sherlockbeard May I ask why? Just curious it seemed to work fine

@sherlockbeard
Copy link
Contributor Author

@nadiration actually it is not working for android tv or tablet . anything with large screen size
#5925 (comment)

@nadiration
Copy link
Contributor

@sherlockbeard This may be fixed later on as phone users are the majority and it's working fine there?? This is really useful feature.

@triallax
Copy link
Contributor

@nadiration just because phone users are the majority does not mean we ignore the rest (even temporarily).

@TobiGr
Copy link
Contributor

TobiGr commented Aug 13, 2021

there is an additional fragment_video_detail.xml in the land layout directory for tablets and TVs. You did not modify it.

@sherlockbeard
Copy link
Contributor Author

sherlockbeard commented Aug 13, 2021

i have decided to open a new pull request and fix the issue . Thanks @TobiGr for pointing me in the right direction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface localisation / translation Everything that has to do with translations or Weblate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "Video not available" thumbnail on unplayable videos needs some changes
9 participants