UX: use "icon-picker" & "image-uploader" fields to set group flair. (PR #9779)

GitHub

This seems good to me, but I’d like @jjaffeux to review - maybe he has a better idea about removing the observer to download the SVG.

why not set here ?

why not set here ?

maybe you can try to replace all of this by @on(“didReceiveAttrs”)

not sure it would work didn’t pull the pr

I think this new upload_id column will need to be added to clean_up_uploads.rb, to make sure the upload doesn’t get deleted automatically.

add_reference is discouraged in our code base now. It’ll be better to be more explicit about the commands here.

Also I think flair_upload_id will be a better name since we’re referencing an upload.

This association seems strange to me. I would think that a Group -> has_one :flair_image rather than belongs_to it

Since we are migrating the icons to their own column, I think this would be a good time to simplify the format as well. Currently, the accepted formats are “fa-icon” or “fas fa-icon” for solid style icons, “far fa-icon” for regular and “fab fa-icon” for brands. This is finicky (and sadly, it’s my fault) and it doesn’t work with the icon picker.

The following one-line change should work:

UPDATE groups SET flair_icon = REPLACE(REPLACE(flair_url, 'fas fa-', ''), ' fa-', '-')

After this, the accepted formats are “icon”, “far-icon” and “fab-icon”. (And the picker enforces the correct format anyway.)

I think we should be careful here, this erases flair_url columns when if you revert the migration for some reason. It might be safer to use up down methods instead.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/ability-to-upload-a-group-image-directly-when-creating-it/151636/3

Since the target table has primary key field (upload.id) and “group” table has child key we should use belongs_to here, I hope. We have a similar code in user.rb file.

Previously, I tried has_one and it returned an error.

All the requested changes are done :+1:

1 Like

Yup you’re right. I got confused about where the foreign key should be on.