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

Added modal to show error in avatar size #26

Merged
merged 1 commit into from
Jul 29, 2023
Merged

Conversation

1bacon
Copy link
Contributor

@1bacon 1bacon commented Jul 27, 2023

Got around to adding a simple modal to communicate the error.

image

The size check is also only client side, meaning it is possible to upload any size with something like this:
$("soci-avatar-uploader")._MINIMUMSIZE = 0 )

Not sure if that's a problem or not.

@1bacon
Copy link
Contributor Author

1bacon commented Jul 27, 2023

Solves jjcm/nonio#156

@jjcm jjcm merged commit 125bd88 into jjcm:master Jul 29, 2023
@jjcm
Copy link
Owner

jjcm commented Jul 29, 2023

This looks solid! Thank you for jumping in and fixing this.

I generally try and use inline errors rather than modals, but given that there's no error at all right now something is definitely better than nothing, so I'm gonna merge this. I'll likely change this a bit once I overhaul that settings page, but it's pretty low on my list right now.

Awesome work!

@jjcm
Copy link
Owner

jjcm commented Jul 29, 2023

Also, do you have a username on nonio I can tag as thanks in the next update?

@1bacon
Copy link
Contributor Author

1bacon commented Jul 29, 2023

I generally try and use inline errors rather than modals, but given that there's no error at all right now something is definitely better than nothing, so I'm gonna merge this. I'll likely change this a bit once I overhaul that settings page, but it's pretty low on my list right now.

Yeah, I tried a couple different configurations, showing the min size even before you try to change it. But nothing looked good enough. "Better than nothing" was exactly what I was going for. You said that you wanted to redo the entire user admin page anyway (+1), so I thought something easy to remove couldn't hurt.

Cheers for merging

@1bacon
Copy link
Contributor Author

1bacon commented Jul 29, 2023

Also I hoped it was obvious but I guess not.
It is me, 1b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants