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

chore: Change time format #7274

Merged
merged 17 commits into from Feb 1, 2024
Merged

Conversation

arian81
Copy link
Contributor

@arian81 arian81 commented Feb 1, 2024

Description

Please include a concise summary, in clear English, of the changes in this pull request. If it closes an issue, please mention it here.

Closes: #7179

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 1, 2024

🪼 branch checks and previews

Name Status URL
Spaces building...
Website building...
Storybook ready! Storybook preview
🦄 Changes detected! Details

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 1, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/audio patch
@gradio/utils patch
@gradio/video patch
gradio patch

With the following changelog entry.

chore: Change time format (thanks @jjshoots for the independent contribution).

⚠️ The changeset file for this pull request has been modified manually, so the changeset generation bot has been disabled. To go back into automatic mode, delete the changeset file.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@abidlabs
Copy link
Member

abidlabs commented Feb 1, 2024

Thanks @arian81! You addressed my suggestion from #7270 which was to extract the logic into a separate helper function, so this looks good.

As mentioned in #7270, there is actually one more instance of time formatting that needs to be fixed, which is here:

return `${minutes}:${_seconds}`;

@abidlabs abidlabs added the v: patch A change that requires a patch release label Feb 1, 2024
@arian81
Copy link
Contributor Author

arian81 commented Feb 1, 2024

Thanks @arian81! You addressed my suggestion from #7270 which was to extract the logic into a separate helper function, so this looks good.

As mentioned in #7270, there is actually one more instance of time formatting that needs to be fixed, which is here:

return `${minutes}:${_seconds}`;

I will change that instance. I had one question tho and that is why the formatting logic for video player different than the rest. Currently i didn't apply the same function to the video player because i'm not even sure which component is using that one. But it would nice if you could clarify this.

@abidlabs
Copy link
Member

abidlabs commented Feb 1, 2024

I believe its the static Video component that is using this. So if you were to do:

import gradio as gr

with gr.Blocks() as demo:
    gr.Video("test.mp4")
    
demo.launch()

you'll see that even with your changes, its still using the mm:ss format right now

@arian81
Copy link
Contributor Author

arian81 commented Feb 1, 2024

I believe its the static Video component that is using this. So if you were to do:

import gradio as gr

with gr.Blocks() as demo:
    gr.Video("test.mp4")
    
demo.launch()

you'll see that even with your changes, its still using the mm:ss format right now

ok i changed that one to use the global function as well. Everything seems to be working now.

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Small nit but. looks good! Thanks @arian81 !

js/utils/src/utils.ts Outdated Show resolved Hide resolved
@arian81
Copy link
Contributor Author

arian81 commented Feb 1, 2024

@abidlabs Something seems to be broken in the upload button causing the functional test to fail but I haven't changed that file.

@abidlabs
Copy link
Member

abidlabs commented Feb 1, 2024

I think that's just a flaky functional test we need to get under control, don't worry about it!

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

LGTM thanks @arian81!

@abidlabs abidlabs merged commit fdd1521 into gradio-app:main Feb 1, 2024
8 checks passed
@pngwn pngwn mentioned this pull request Feb 1, 2024
@jjshoots
Copy link

jjshoots commented Feb 1, 2024

Thanks for this everyone! Needed this to get something blocking on my end moving. :)

aliabid94 pushed a commit that referenced this pull request Feb 4, 2024
* fix: typos

* fix: remove newlines from latex delimiters

* add changeset

* chore: change time format to HH:MM:SS

* add changeset

* Revert "fix: remove newlines from latex delimiters"

This reverts commit cebfabb.

* fix: remove accidental changes from different pr

* add changeset

* chore: change time format for player to use global formatting function

* fix: remove pipfile

* chore: use global time formatting for static video player

* chore: change to snake_case

* change

---------

Co-authored-by: gradio-pr-bot <gradio-pr-bot@users.noreply.github.com>
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v: patch A change that requires a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HH:MM:SS time code in gr.Audio
5 participants