First of all, thank you for taking the time to do this post-merge review
@gravitystorm . I wish these points had been raised earlier so I could have
reassessed and addressed them before merging.
I completely agree with your observations. As mentioned earlier, I drew some
inspiration from the rails library mentioned in the issue. The
`filter_allowed_sites` method was intended to validate a given list of sites
and render only the valid ones. If no list was provided, it would default to
rendering all sites, assuming that social share buttons without any provider
wouldn’t be practical. That said, I agree the implementation could have been
cleaner and better aligned with our use case.
I noticed that @tomhughes has already opened #5417 to address this. I wish i
was active at the time issue was raised to do it instead. Never the less, I
appreciate the feedback and will keep this in mind as we refactor and finalize
social sharing functionality.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/5415#issuecomment-2552446197
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/issues/5415/2552446...@github.com>
_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
