Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Containerized branch based on 303-proxy_http_ffmpeg_streams #305
base: master
Are you sure you want to change the base?
Containerized branch based on 303-proxy_http_ffmpeg_streams #305
Changes from 1 commit
fbf466d
356d5b7
e0be54b
f4abfbf
a66807c
678bd31
cb79dcd
8fc8e5c
489cbf8
f93ba6d
30e87c5
d5fc8d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer for this to remain left-aligned so it's less likely to cover karaoke video text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do, not a biggie. It looked a little awkward when I had a long url there... I have one instance spun up on my personal server, and I know it would look odd to have the QR code left aligned.... buuuut even centered, the long URL + the QR doesn't look the greatest graphically either.
Been pondering a way to make it look more visually appealing... or maybe make the url text disappear altogether once a certain character count is reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree its not great either way, maybe lowering the text size of the url would make it look more symmetrical. Or it might be worth adding a command line option to hide the url, but show the QR code, for minimalists. If that were the case a corner-aligned QR code would look pretty clean. But that's another ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be worth adding as a tick box option in the admin area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look and I'm not crazy about these boxes. They cover up much more screen real estate than the text alone, much of it being blank space. If the goal is to make the text more readable, I feel that adding a black or semi-transparent stroke effect to them would achieve the same thing and minimize how much of the video this is all covering up. https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-text-stroke
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, stroke is not great because it's an internal, not external stroke so it makes the text thinner. But an 8-axis shadow should work:
https://stackoverflow.com/a/47511171/2909171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... I really like the look, to me it makes the interface very polished looking, and on most my screens it's rare they cover up important words.
I think what I might try to do is configure this so by default it's off, but ad a check box in the admin area so it can be turned on and off dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the general look, I think it's just the excessive blank space that bugs me. Maybe if the singer name goes on the same line as the song title, it would be cleaner. Then give it a max-width so it will wrap if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, I don't think it's trivial to add admin settings checkboxes that will affect client side frontend. You'd probably have to dip into flask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought, maybe the project in general should allow css overrides for custom "skinning". There's no way any interface will make everyone happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I'm definitely feeling that with the QR code area at the moment.