-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix(subtitles): Styles. #15043
fix(subtitles): Styles. #15043
Conversation
hristoterezov
commented
Aug 22, 2024
- Move the styles from css to tss-react ones
- Dynamic fontSize based on the visible area of the page
- Remove the gaps in the background when a line is wrapped.
- Change the text color to white.
- Remove transparency.
*/ | ||
export function calculateSubtitlesFontSize(clientHeight?: number) { | ||
if (typeof clientHeight === 'undefined') { | ||
return 16; |
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.
Extract to a constant since it's used in 2 places here?
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.
Sure!
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.
fixed
1px 0px 1px rgba(0,0,0,0.3), | ||
0px 0px 1px rgba(0,0,0,0.3)`, | ||
transform: 'translateX(-50%)', | ||
zIndex: 7, |
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.
Why 7? Is it relative to something? A comment explaining woould help.
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.
Probably. Now the thing is this is a copy paste of the css :)
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.
Do you insist on reviewing all z-indexes so that we can write what do we want to cover?
If we are doing this wouldn't it be more beneficial, to review all zIndex levels, to define all z-index levels as variables with meaningful names and comments?
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.
Added a comment for now
|
||
return { | ||
transcriptionSubtitles: { | ||
bottom: _isLifted ? `${toolbarSize + 36 + 40}px` : `${toolbarSize + 40}px`, |
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.
Where do those 36 and 40 come from?
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.
ditto.
For 36
I guess the intention was to lift it for the label for the on stage participant's name. Will add a comment or something.
For 40
I can only guess this only the desired padding...
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.
Now I see that this whole thing is kind of dependent on the onstage participant display name badge, which uses the theme. I'll move those in a common place and use it from there.
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 should be fixed now. both stage participant display name badge and the captions are using the same thing from a generic function which uses the theme.
29f7f35
to
9548504
Compare
- Move the styles from css to tss-react ones - Dynamic fontSize based on the visible area of the page - Remove the gaps in the background when a line is wrapped. - Change the text color to white. - Remove transparency.
9548504
to
f1d4b50
Compare