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

Remove Iconify from FeedInfoFragment #6655

Merged
merged 5 commits into from Nov 22, 2023
Merged

Remove Iconify from FeedInfoFragment #6655

merged 5 commits into from Nov 22, 2023

Conversation

caoilte
Copy link
Contributor

@caoilte caoilte commented Sep 24, 2023

Part of #5925
Using the same compound drawable pattern I used in #6578 for disk full in navigation tile

Feed Info Before

current_paperclip

Feed Info After

new_paperclip

ByteHamster
ByteHamster reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@ByteHamster ByteHamster left a comment

Choose a reason for hiding this comment

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

Thanks! By the way, where did you get the new icon from?

core/src/main/res/drawable/ic_paper_clip.xml Outdated Show resolved Hide resolved
@caoilte
Copy link
Contributor Author

caoilte commented Sep 28, 2023 •
edited

Thanks! By the way, where did you get the new icon from?

https://icon-sets.iconify.design/fa-solid/paperclip/

Also takes a bit of fiddling in gimp and other tools to extract the paths and then to reduce their complexity.

@ByteHamster
Copy link
Member

I would prefer to use these icons if possible, because we already attribute them on our licenses page: https://pictogrammers.com/library/mdi/

@caoilte
Copy link
Contributor Author

caoilte commented Sep 28, 2023

I would prefer to use these icons if possible, because we already attribute them on our licenses page: https://pictogrammers.com/library/mdi/

Sure. I'll redo the disc full icon using that source as well.

Are you okay with with paper clip icon no longer being diagonal as a result?

https://pictogrammers.com/library/mdi/icon/paperclip/

@ByteHamster
Copy link
Member

Are you okay with with paper clip icon no longer being diagonal as a result?

Sure :)

@caoilte
Copy link
Contributor Author

caoilte commented Oct 1, 2023 •
edited

More updated screenshots. disc full is slightly larger. looked slightly better slightly larger than slightly smaller.

Feed Info with new vertical paperclip

new_paperclip_vertical

New Disc Full icon

new_disc_full

ByteHamster
ByteHamster reviewed Oct 2, 2023
View reviewed changes
Copy link
Member

@ByteHamster ByteHamster left a comment

Choose a reason for hiding this comment

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

Thanks! I have some comments :)

core/src/main/res/drawable/ic_paperclip.xml Outdated
android:scaleY="0.5">

<path
android:fillColor="#000000"
Copy link
Member

Choose a reason for hiding this comment

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

Please use ?attr/action_icon_color, so it is visible in the dark theme

core/src/main/res/drawable/ic_disc_alert.xml Outdated
android:viewportHeight="24">

<path
android:fillColor="#000000"
Copy link
Member

Choose a reason for hiding this comment

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

Please use ?attr/action_icon_color, so it is visible in the dark theme

core/src/main/res/drawable/ic_paperclip.xml Outdated
android:viewportHeight="12">

<group
android:scaleX="0.5"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Can't you just adapt the viewport to scale it? That simplifies the icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the delay on this. Was away for a while and then took another while to get caught up.

AFAICT the viewport attributes only change the amount of the icon that is visible, ie they crop it and setting it to 6/6 ends up only showing the top left of the paperclip.

I had a look around online and couldn't see any obvious other solution short of halving all of the numbers in the source icon pathData attribute.

Copy link
Member

Choose a reason for hiding this comment

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

I would have removed the scale and set the viewport to 24/24, not to 6/6

Copy link
Member

Choose a reason for hiding this comment

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

@caoilte does setting the viewport to 24/24 and removing the scaling work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaaah. neat! yeah that did the trick! thanks for explaining.

@ByteHamster ByteHamster added the Needs: Reply Issue or PR is awaiting follow-up, as requested by project maintainers. label Oct 22, 2023
@ByteHamster ByteHamster removed the Needs: Reply Issue or PR is awaiting follow-up, as requested by project maintainers. label Oct 29, 2023
@ByteHamster ByteHamster added the Needs: Reply Issue or PR is awaiting follow-up, as requested by project maintainers. label Nov 18, 2023
@ByteHamster ByteHamster removed the Needs: Reply Issue or PR is awaiting follow-up, as requested by project maintainers. label Nov 22, 2023
@ByteHamster ByteHamster merged commit 95f431f into AntennaPod:develop Nov 22, 2023
7 checks passed
@ByteHamster
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ByteHamster ByteHamster ByteHamster left review comments

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@caoilte @ByteHamster