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

PB-302: Add icon size to metadata #81

Merged
merged 9 commits into from
Sep 4, 2024

Conversation

LukasJoss
Copy link
Contributor

No description provided.

@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch 2 times, most recently from bd363b5 to ce2e83f Compare August 28, 2024 13:32
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

Before merging this, we should change the CI to use the new codebuild project way (so we can test it with this PR 😉 ) it is a very quick task, see PB-837 ticket. You can see this 2 recent PR were I did this work for qrcode;

Beside that, in a separate PR we should also update python to 3.12 and all its dependencies before re-deploying service-icons (only do this if no big issues with newer version but I don't expect any) so we keep the service most up to date.

app/routes.py Outdated Show resolved Hide resolved
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch from 263cfba to 47677ea Compare August 29, 2024 12:26
app/helpers/icons.py Outdated Show resolved Hide resolved
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch 2 times, most recently from 8367096 to 6472c5f Compare August 30, 2024 13:31
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch from 6472c5f to 1832ea4 Compare August 30, 2024 13:35
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch from 1832ea4 to d0834d4 Compare September 2, 2024 07:06
@LukasJoss LukasJoss requested review from ltshb, hansmannj and pakb and removed request for hansmannj September 2, 2024 15:46
app/routes.py Outdated
Comment on lines 97 to 98
#new_size = 48
#image = image.resize((new_size[0], new_size[1]))
Copy link
Member

Choose a reason for hiding this comment

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

Those 2 lines can be deleted I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

What about for request with scale bigger than 1, the service should resize (even though currently not used).

Copy link
Member

@hansmannj hansmannj left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.
Only one thing: we should have a kind of versioning in the path.
We cannot change the path of the old icon sets, for backwards compatibility.
But for the new sets, we should add a v2 or a timestamp, e.g. 2024-09 or the like to the path.

Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

Just implement the case where scale is bigger than 1.

@ltshb
Copy link
Contributor

ltshb commented Sep 3, 2024

Thanks, LGTM. Only one thing: we should have a kind of versioning in the path. We cannot change the path of the old icon sets, for backwards compatibility. But for the new sets, we should add a v2 or a timestamp, e.g. 2024-09 or the like to the path.

I think this is out of scope of this PR and needs to be refined.

@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch 2 times, most recently from 2ddacb4 to 6c52f00 Compare September 3, 2024 14:15
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch from 6c52f00 to 616f25b Compare September 3, 2024 14:21
@LukasJoss LukasJoss requested a review from ltshb September 3, 2024 14:27
app/routes.py Outdated
new_size = int(48 * scale)
if new_size != icon_set.get_default_pixel_size():
if scale != 1:
new_size = int(DEFAULT_ICON_SIZE * scale)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you should take the size from your function not the hardcoded 48px, so the code works if we have in future non 48px icons and also if the icon are not squared:

if scale != 1:
  new_size = icon.get_size().map(s => s * scale)
  image = image.resize(new_size)

@LukasJoss LukasJoss requested a review from ltshb September 4, 2024 07:22
@LukasJoss LukasJoss force-pushed the feat-PB-302-add-icon-size-to-metadata branch from f2aeee8 to 5af89c1 Compare September 4, 2024 08:12
@LukasJoss LukasJoss merged commit d9fc68b into develop Sep 4, 2024
3 checks passed
@LukasJoss LukasJoss deleted the feat-PB-302-add-icon-size-to-metadata branch September 4, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants