-
Notifications
You must be signed in to change notification settings - Fork 2
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
Various video services fix and added e2e testing #552
Conversation
e757d2a
to
3733b6c
Compare
f74c5d3
to
9046762
Compare
I'm aware that the PR is becoming quite large so will stop here. |
I will have a look next week. I browsed the changes quickly and saw that you added an icon. Could you use MUI's icon instead? You can use the Download or FileDownload icon from MUI for instance. Link to the documentation Side note, we have a few custom icons we use, and in case you really need one, you can have a look at what I recently did for the tools refactoring in the tools.jsx file (in beta-master). |
There are some icons that don't have a direct equivalent like the keyframes or the forensic icon. Do you have a guideline on what you plan to be using? |
I've made it optional so that both the image file and svg can be used for now with the intention of replacing all the icons to svg eventually. |
Sorry @twinkarma I think my message was confusing, if there is no equivalent, feel free to use a custom SVG (SVG being better than JPEG because we can embed it in an MUI component and have consistent styling). |
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.
The PR looks good to me, just a small requested change due to the tools refactoring and the introduction of constants files
}; | ||
|
||
const MediaServices = { | ||
analysisVideo: "navbar_analysis_video", |
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.
To avoid having hardcoded strings, I introduced a constants folder in which there is the tools file. Please use this as this will be used in the codebase moving forward. This will also make your life easier if some keys change in the future
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 you provide me a link to this constants folder so I can use the existing files as an example?
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.
Yes, you can reuse the constants in src/constants/tools.jsx
i.e. you can grab the titleKeyword
(first property) of the videoAnalysis object:
const videoAnalysis = new Tool(
"navbar_analysis_video",
"navbar_analysis_description",
AnalysisSvgIcon,
TOOLS_CATEGORIES.VIDEO,
null,
null,
"analysis",
TOOL_GROUPS.VERIFICATION,
<Analysis />,
<Footer type={FOOTER_TYPES.ITI} />,
);
(you will need to export the object)
https://github.com/AFP-Medialab/verification-plugin/blob/beta-master/src/constants/tools.jsx
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.
@Sallaa unfortunately there's no easy way to import .jsx into normal javascript for running the playwright e2e tests. Also even if it's possible the constants file has a lot of component imports which will introduce unnecessary dependencies into the test file.
I think it's ok to keep the this list in the test separate from the constants file and if the keys should change in the app (which shouldn't be often), it will be flagged up in the tests anyway.
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'm looking into updating the AssistantRuleBook to use the new constants, I just wanted to check with you how I can access a specific tool.
Since all the tools inside the tools
object is stored an array, to get a specific tool I'd need to iterate through each one and search for the right one? Also since there's no unique ID assigned to each tool, the only thing I can use to find the correct one is the titleKeyword
or descriptionKeyword
field. Would it not be better to store the tools list as a dictionary instead?
So we can use it like this (from AssistantRuleBook.jsx
):
export const ASSISTANT_ACTIONS = [
{
title: tools.videoAnalysis.titleKeyword,
icon: tools.videoAnalysis.icon,
linksAccepted: [KNOWN_LINKS.YOUTUBE, KNOWN_LINKS.FACEBOOK],
cTypes: [CONTENT_TYPE.VIDEO],
exceptions: [],
useInputUrl: true,
text: tools.videoAnalysis.descriptionKeyword,
tsvPrefix: "api",
path: tools.videoAnalysis.path,
},
Or is the plan to add the properties of the objects in ASSISTANT_ACTIONS into the constants file itself?
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.
Ah, sorry, I thought you were using a JSX file -- feel free to leave it as it is. The goal is not to add bulk and complexity
It is fine for me if you export a specific tool constant from the tools so that you do not need to iterate through the array. I have exported some of the tools already.
Speaking about the array, yes, it makes more sense to use some kind of a dictionary instead, I'll update this when I'll have some more time to continue the refactoring. I think I have an instance where I need to search by titleKeyword
but it's not ideal at all. 🥲
Are you updating the AssistantRuleBook
in this PR? Just to know if I can merge this or if I should wait a little more
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 PR is ready to go, but waiting for some translation checking in AFP-Medialab/InVID-Translations#90. We need French, Italian and German.
I might as well wait to update the AssistantRuleBook
when you've refactored the constants file.
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.
Looks good!
60a8b34
to
e479d29
Compare
e479d29
to
eedfc10
Compare
blob: