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.

Sign up for GitHub

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

Jump to bottom

fix(VTT): Fix spacing between text lines #4961

Merged
merged 3 commits into from
Feb 10, 2023
Merged

fix(VTT): Fix spacing between text lines #4961

merged 3 commits into from
Feb 10, 2023

Conversation

avelad
Copy link
Member

@avelad avelad commented Feb 3, 2023

Fixes #4958

@avelad avelad added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround component: WebVTT The issue involves WebVTT subtitles specifically labels Feb 3, 2023
@avelad avelad mentioned this pull request Feb 3, 2023
Closed
@avelad avelad requested a review from joeyparrish February 3, 2023 09:46
@avelad
Copy link
Member Author

avelad commented Feb 3, 2023

@joeyparrish , I have tested the change manually and it works as expected, could you help me update the test images?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

Incremental code coverage: 100.00%

@joeyparrish
Copy link
Member

I was just writing a detailed explanation of how to go about updating the screenshots, which involves an automated workflow and about 10 manual steps afterward... and now I realize it could all be automated, assuming we have already verified the screenshot changes.

I'm going to build a new workflow to update all screenshots in a PR from the artifacts of the lab workflow.

@joeyparrish
Copy link
Member

I'm going to build a new workflow to update all screenshots in a PR from the artifacts of the lab workflow.

I couldn't quite finish it last night, but I'm testing it now and I expect I'll have it working within the next hour or two. (The lab is often slow.)

@joeyparrish
Copy link
Member

The status reporting is broken, but the rest of the workflow is working. This PR was updated by the workflow and should pass tests now.

joeyparrish
joeyparrish previously approved these changes Feb 10, 2023
View reviewed changes
@joeyparrish joeyparrish changed the title fix: Fix spacing between text lines when any styling present in the webvtt cue fix(VTT): Fix spacing between text lines Feb 10, 2023
@joeyparrish
Copy link
Member

I updated related comments on the tests. The screenshots involving bold, italic, and underline text were known-bad, and those tests had comments about the gaps between lines.

@avelad
Copy link
Member Author

avelad commented Feb 10, 2023

Thanks @joeyparrish !

@avelad
Copy link
Member Author

avelad commented Feb 10, 2023

@joeyparrish , there's a problem with the CLA, can you review it? Thanks!

@joeyparrish
Copy link
Member

The CLA issue is because of the commit made by the default GitHub Actions bot. It's fine. I'll update the workflow with a PAT to fix that later.

@joeyparrish joeyparrish merged commit 2d0469f into shaka-project:main Feb 10, 2023
@github-actions github-actions bot mentioned this pull request Feb 10, 2023
Merged
@avelad avelad deleted the spacing-between-text branch February 11, 2023 18:04
joeyparrish added a commit that referenced this pull request Mar 1, 2023
@avelad @github-actions @joeyparrish
fix(VTT): Fix spacing between text lines (#4961)
1194d74 
Fixes #4958

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joey Parrish <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 1, 2023
Merged
joeyparrish added a commit that referenced this pull request Mar 1, 2023
@avelad @github-actions @joeyparrish
fix(VTT): Fix spacing between text lines (#4961)
2bf8526 
Fixes #4958

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joey Parrish <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 1, 2023
Merged
joeyparrish added a commit that referenced this pull request Mar 1, 2023
@avelad @github-actions @joeyparrish
fix(VTT): Fix spacing between text lines (#4961)
a44fe84 
Fixes #4958

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joey Parrish <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 1, 2023
Merged
joeyparrish added a commit that referenced this pull request Mar 2, 2023
@avelad @github-actions @joeyparrish
fix(VTT): Fix spacing between text lines (#4961)
40bff87 
Fixes #4958

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joey Parrish <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 2, 2023
Merged
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

Assignees
No one assigned
Labels
component: WebVTT The issue involves WebVTT subtitles specifically priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spacing between text lines when any styling present in the webvtt cue
2 participants
@avelad @joeyparrish