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

Define a face style #83

Merged
merged 1 commit into from
Apr 18, 2022
Merged

Define a face style #83

merged 1 commit into from
Apr 18, 2022

Conversation

atjn
Copy link
Contributor

@atjn atjn commented Apr 18, 2022

Hi and thanks for making this very cool addition to owntracks!

Currently, the size of a face image in a popup is determined by the size of the image uploaded by the user. This can make popups look wonky because their layout will change based on this arbitrary image size, and it can even break the layout completely with a sufficiently large face:
Screenshot from 2022-04-18 02-39-35

This change fixes the images to a specific size, no matter their original resolution:
Screenshot from 2022-04-18 03-16-18

While I was there, I also took the liberty of rounding the corners a bit. I think it looks much nicer that way :)

I am not sure which image sizes are officially allowed right now, but I have opened a discussion to allow images of any size over at owntracks/talk#138, and if that change is accepted, then this fix will be pretty important.

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Thanks! Given the defacto default is 40x40 px currently, let's not use a larger default size. You can use max-width/height to still limit the maximum size without imposing a minimum size. (Please squash the commits when making that change.)

I also don't think the slightly rounded corners fit the rest of the UI very well, most elements with a border radius use 100%.

@atjn
Copy link
Contributor Author

atjn commented Apr 18, 2022

Given the defacto default is 40x40 px currently, let's not use a larger default size. You can use max-width/height to still limit the maximum size without imposing a minimum size.

I considered using max-width, but that would still allow layout shift if images are smaller than 60px. Maybe the best course of action is to use a fixed 40px width right now, and then if owntracks/talk#138 is accepted (which recommends 192px source images), we could update that to a larger fixed size sometime in the future?

I also don't think the slightly rounded corners fit the rest of the UI very well, most elements with a border radius use 100%.

I initially wanted to do that, but then I saw some of your example images and noticed that you are cleverly using transparent PNGs to make the image look much more integrated in the popup. If I set a 100% border rounding, parts of your iPad would be cut off, and it would look pretty weird. This can be solved-ish by making a visible border around the image, thus showing that it is round, and making it clear why parts have been cut off, but that would make it impossible to have a sleek integration like the one you show in the example.

Because of that, I think the slightly rounded corners are the best solution. The popup container is an example of an element that has slightly rounded corners. Maybe you would like it more if I match it so that the image and popup has the same amount of rounding? (I actually really like the slightly larger rounding on the image, but I will accept a compromise 😉)

@linusg
Copy link
Member

linusg commented Apr 18, 2022

but that would still allow layout shift if images are smaller than 60px.

Not sure I understand - it works now, why doesn't it work when setting a maximum size? (Which I'm happy to add regardless of the other issue's outcome)

Maybe you would like it more if I match it sothat the image and popup has the same amount of rounding?

I think that would be a good compromise then, yes :)

@atjn
Copy link
Contributor Author

atjn commented Apr 18, 2022

Not sure I understand - it works now, why doesn't it work when setting a maximum size? (Which I'm happy to add regardless of the other issue's outcome)

For example, if you only set a max-size: 60px, and your users have a mix of images that are either sized 40px or 192px, then the popups will have different image sizes and different layout depending on which image size is available for that particular user, and IMO that makes the interface design feel a bit random and badly designed. It feels the same way as it would if every popup had a random font size.
The only way I see to get around that problem, is to have a fixed image size that is always used no matter what the original resolution was.

@atjn
Copy link
Contributor Author

atjn commented Apr 18, 2022

I have updated the PR to use a matched border rounding, and to use fixed 40px sizing for the image, so if you agree with me on that last one, then it is ready to merge :)

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Perfect, let's do it like this then and revisit once can get consensus that larger images should generally be accepted. Thanks again!

@linusg linusg merged commit b2273c0 into owntracks:main Apr 18, 2022
@linusg linusg added Type: Enhancement Improve something that's already there Status: Done Feature has been implemented, bug fixed, question answered etc. labels Apr 18, 2022
@atjn atjn deleted the face-design branch April 18, 2022 20:07
@jpmens
Copy link
Member

jpmens commented Apr 20, 2022

@linusg I think @atjn is right with his suggestion for 192x192 images, and unless somebody steps in with objections, we'll probably settle on those as max dimensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Done Feature has been implemented, bug fixed, question answered etc. Type: Enhancement Improve something that's already there
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants