Refactor placeholder logo into common controller #673

2024-04-25T16:41:29+08:00

JoshAntBrown commented

2024-04-25 16:41:29 +08:00

(Migrated from github.com)

We're starting to see a lot of duplication around account logo placeholders, and there's already a couple of inconsistencies.

This PR adds a way of rendering an SVG placeholder logo on a per account basis so that we can simply just render the SVG.

This can be updated in the future to redirect/pull from third party sources in future, with the option of always falling back to the placeholder if there are any failures.

We're starting to see a lot of duplication around account logo placeholders, and there's already a couple of inconsistencies. This PR adds a way of rendering an SVG placeholder logo on a per account basis so that we can simply just render the SVG. This can be updated in the future to redirect/pull from third party sources in future, with the option of always falling back to the placeholder if there are any failures.

zachgoll (Migrated from github.com) reviewed 2024-04-25 19:47:33 +08:00

zachgoll (Migrated from github.com) left a comment

I like this idea and definitely agree that this refactor is needed.

The only portion of this that I'll throw out a question for is the strategy we're using to deliver them. Below was the route I was thinking we'd go for this:

Use ActiveStorage for the logo (account.logo)
By default, account.logo is nil, and we render the placeholder SVG here in this PR
We use a Account::Provided module, similar to how we provide an exchange rate to provide an external logo from a provider like Synth (i.e. logos endpoint)
- Our "caching" strategy here would probably be to save the provided logo directly to the DB (account.logo) and never have to fetch it again

Any thoughts on moving towards this type of strategy? Anything I'm overlooking here?

I like this idea and definitely agree that this refactor is needed. The only portion of this that I'll throw out a question for is the _strategy_ we're using to deliver them. Below was the route I was thinking we'd go for this: - Use ActiveStorage for the logo (`account.logo`) - By _default_, `account.logo` is `nil`, and we render the placeholder SVG here in this PR - We use a `Account::Provided` module, similar to how we [provide an exchange rate](https://github.com/maybe-finance/maybe/blob/main/app/models/exchange_rate/provided.rb) to provide an external logo from a provider like Synth (i.e. [logos endpoint](https://docs.synthfinance.com/reference/logos)) - Our "caching" strategy here would _probably_ be to save the provided logo directly to the DB (`account.logo`) and never have to fetch it again Any thoughts on moving towards this type of strategy? Anything I'm overlooking here?

JoshAntBrown commented

2024-04-25 20:45:29 +08:00

(Migrated from github.com)

I think we could implement that strategy through the controller.

I've been borrowing this approach from the Campfire code where they do something similar for avatars.

If we wanted to store the logos in ActiveStorage we could add the following to the controller to stream them through:

include ActiveStorage::Streaming
def send_webp_blob_file(key)
  send_file ActiveStorage::Blob.service.path_for(key), content_type: "image/webp", disposition: :inline
end

This also avoids the complexity of having to store a placeholder SVG in ActiveStorage, our controller can just check if the logo exists and if not then render it on demand. We perform regular caching here if required.

I think we could implement that strategy through the controller. I've been borrowing this approach from the Campfire code where they do something similar for avatars. If we wanted to store the logos in ActiveStorage we could add the following to the controller to stream them through: ```ruby include ActiveStorage::Streaming def send_webp_blob_file(key) send_file ActiveStorage::Blob.service.path_for(key), content_type: "image/webp", disposition: :inline end ``` This also avoids the complexity of having to store a placeholder SVG in ActiveStorage, our controller can just check if the logo exists and if not then render it on demand. We perform regular caching here if required.

zachgoll commented

2024-04-25 21:55:23 +08:00

(Migrated from github.com)

@JoshAntBrown I wasn't saying we'd be storing a placeholder in ActiveStorage, we'd just do something like this:

views/accounts/_logo.html.erb:

<%# locals: (account:, size:) %>

<% if account.logo.attached? %>
  <%= image_tag account.logo, size: size, alt: "#{account.name} Logo" %>
<% else %>
  Custom placeholder SVG
<% endif %>

The Campfire approach makes sense to me for user-uploaded content like avatars where you'd be getting various sizes and cache lifetimes, I guess I was just questioning whether we'd need all that fine-grained control (for rarely changing , optimized account logos) and whether we'd be okay with the number of network requests this introduces (which may be fine).

Either way, my main concern is making sure we can easily keep this aligned with our "provided" 3rd party data strategy, which looks like we can.

@JoshAntBrown I wasn't saying we'd be storing a placeholder in ActiveStorage, we'd just do something like this: `views/accounts/_logo.html.erb`: ```erb <%# locals: (account:, size:) %> <% if account.logo.attached? %> <%= image_tag account.logo, size: size, alt: "#{account.name} Logo" %> <% else %> Custom placeholder SVG <% endif %> ``` The Campfire approach makes sense to me for user-uploaded content like avatars where you'd be getting various sizes and cache lifetimes, I guess I was just questioning whether we'd need all that fine-grained control (for rarely changing , optimized account logos) and whether we'd be okay with the number of network requests this introduces (which may be fine). Either way, my main concern is making sure we can easily keep this aligned with our "provided" 3rd party data strategy, which looks like we can.

JoshAntBrown commented

2024-04-25 22:18:33 +08:00

(Migrated from github.com)

Ah, sorry, yes that makes sense.

The partial approach might be simpler in some ways for right now given we only have the placeholder, though I was anticipating we might want to call a provider or something at some point, and that might require some more complex logic that might be better suited in a controller.

Hopefully with HTTP2, and appropriate caching the impact of additional network requests should be mitigated too.

I'd say an endpoint is also a bit more flexible / future-proof if at some point we wanted to render an account logo in an email or something, though happy to go down the partial route for the time being.

Ah, sorry, yes that makes sense. The partial approach might be simpler in some ways for right now given we only have the placeholder, though I was anticipating we might want to call a provider or something at some point, and that might require some more complex logic that might be better suited in a controller. Hopefully with HTTP2, and appropriate caching the impact of additional network requests should be mitigated too. I'd say an endpoint is also a bit more flexible / future-proof if at some point we wanted to render an account logo in an email or something, though happy to go down the partial route for the time being.

zachgoll commented

2024-04-25 22:32:21 +08:00

(Migrated from github.com)

@JoshAntBrown all good, I don't see a super compelling reason to rework anything at the moment—just wanted to make sure I understood how we'd approach those future changes.

Definitely a good cleanup here so will get it merged!

@JoshAntBrown all good, I don't see a super compelling reason to rework anything at the moment—just wanted to make sure I understood _how_ we'd approach those future changes. Definitely a good cleanup here so will get it merged!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#673