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

fix(quantic): fixed display of timestamps in youtube templates #4676

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mmitiche
Copy link
Contributor

@mmitiche mmitiche commented Nov 14, 2024

SFINT-5816

  • Improved how the time stamps are displayed in the youtube result templates.
  • Also removed some js doc comments that were added with the @type attribute as these are ignored by our doc script and were causing warnings when running the script.

Before

Screenshot 2024-11-07 at 7 21 07 AM

After

Screenshot 2024-11-13 at 4 07 45 PM

@mmitiche mmitiche requested a review from a team as a code owner November 14, 2024 12:10
Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 236.8 236.8 0
commerce 341.5 341.5 0
search 412.8 412.8 0
insight 402.1 402.1 0
recommendation 249.1 249.1 0
ssr 406.3 406.3 0
ssr-commerce 353.7 353.7 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

Comment on lines 325 to 344
getYoutubeFormatTimestamp() {
const hours = Math.floor(this.getHours());
const minutes = Math.floor(this.getMinutes()) % 60;
const seconds = Math.floor(this.getSeconds()) % 60;
let timestamp = '';

const formattedSeconds = seconds < 10 ? '0' + seconds : seconds;

if (hours > 0) {
const formattedMinutes = minutes < 10 ? '0' + minutes : minutes;
timestamp += hours + ':' + formattedMinutes + ':' + formattedSeconds;
} else {
const formattedMinutes =
minutes === 0 ? '0' : minutes < 10 ? '0' + minutes : minutes;
timestamp += formattedMinutes + ':' + formattedSeconds;
}

return timestamp;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Here you could probably use string literals and a helper function to simplify this logic and avoid having to declare formattedMinutes twice. What do you think?

Suggested change
getYoutubeFormatTimestamp() {
const hours = Math.floor(this.getHours());
const minutes = Math.floor(this.getMinutes()) % 60;
const seconds = Math.floor(this.getSeconds()) % 60;
let timestamp = '';
const formattedSeconds = seconds < 10 ? '0' + seconds : seconds;
if (hours > 0) {
const formattedMinutes = minutes < 10 ? '0' + minutes : minutes;
timestamp += hours + ':' + formattedMinutes + ':' + formattedSeconds;
} else {
const formattedMinutes =
minutes === 0 ? '0' : minutes < 10 ? '0' + minutes : minutes;
timestamp += formattedMinutes + ':' + formattedSeconds;
}
return timestamp;
}
getYoutubeFormatTimestamp() {
const format = (num) => (num < 10 ? '0' + num : num);
const hours = Math.floor(this.getHours());
const minutes = Math.floor(this.getMinutes()) % 60;
const seconds = Math.floor(this.getSeconds()) % 60;
const formattedSeconds = format(seconds);
const formattedMinutes = format(minutes);
if (hours > 0) {
return `${hours}:${formattedMinutes}:${formattedSeconds}`;
} else {
return `${minutes === 0 ? '0' : formattedMinutes}:${formattedSeconds}`;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be yeah, I don't see any impact in going one way or another,
personally, not a big fan of declaring a function inside a method tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

then you could declare it outside X) I just think its a bit more readable IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a class we are dealing with, declaring it outside as a method? no we don't wont expose that in this class.
Declaring it as an orphan function in the file? could be but what does it add?
Anyway we are having a discussion about an extremely minor thing, that mainly boils down to preference but does not have a real impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also as a general rule, I don't think minor things like this could be stoppers to not approve PRs,
mainly to avoid doing a lot of back and forth on PRs before getting them merged.

If there are things impactful that can be improved yes of course they are the most welcome,
For very minor things where doing something can be done using method A or method B but without a real impact if any, yes they also are the most welcome to suggest, but should not be stoppers from approving PRs.
That's my opinion and that's the logic I follow at least to approve PRs or not,

Copy link
Contributor

@SimonMilord SimonMilord Nov 14, 2024

Choose a reason for hiding this comment

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

I was mostly kidding for the function, but I agree with you that functions inside of class methods are usually not the best practice. In this case however, it was such a small and simple function that I thought it was making the method more readable than not.

Anyways, as you said, this is definitely not a blocker for me, but I do think of readability as being impactful.

Copy link
Collaborator

@erocheleau erocheleau left a comment

Choose a reason for hiding this comment

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

Can't believe it's not easier than this in JS to convert ms to timestamp haha
well done

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.

3 participants