-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Refactor task output details #280
Conversation
FL-1175 Show details of task outputs in the front
ContextTask outputs permission and transient property are not shown today in the frontend: More information on Design Outputs in Substra frontend Notion page. SpecificationMockups → Task information improvements Task Information Panel - Task information improvements (Figma) User story As a data Scientist or R&D member, I want to understand the tasks, so I need to know all details of the tasks that are processed in a project. Technical specs Input and output line properties Properties are always in the same order. Download and Copy/paste button are always on the same vertical alignment, same for the chevron. Left alignment :
Right :
Icon Set Different
Sub elements (only for Inputs) Default view : → → sub-elements are hidden Capture d’écran 2023-09-14 à 18.01.33.png Clicking on line or arrow: → sub-elements are listed by their filename → sub-elements have no available action Capture d’écran 2023-09-14 à 18.01.19.png Transient property → Text display for incompleted tasks : → " Capture d’écran 2023-09-14 à 17.27.26.png Text display for completed tasks : → " Capture d’écran 2023-09-14 à 17.27.36.png Hovering properties
→ tooltip " Capture d’écran 2023-09-14 à 15.40.55.png
→ tooltip “ Capture d’écran 2023-09-15 à 09.50.23.png
→ tooltip “ Capture d’écran 2023-09-15 à 11.13.36.png
→ tooltip “ Capture d’écran 2023-09-14 à 15.01.50.png
→ tooltip “ Capture d’écran 2023-09-14 à 17.20.06.png General Section Adapt the vertical alignment on the right for the buttons |
201b5e9
to
a1cd868
Compare
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 some small comments to lighten the code
const TaskIOPermissions = ({ | ||
permissions, | ||
}: { | ||
permissions?: PermissionsT | null; |
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.
Not mandatory: instead of defining the prop types inline, you could define them separately for better readability and reusability if more come:
type TaskIOPermissionsProps = {
permissions?: PermissionsT | null;
};
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.
True. I made the choice to not declare a type as it is only used here and it only contains permissions 👍
}: { | ||
permissions?: PermissionsT | null; | ||
}): JSX.Element => { | ||
let label = ''; |
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 label calculation could be wrapped in a useMemo
hook. This would ensure that the label is only recalculated when the permissions prop changes, potentially improving performance.
To be tested but it could be something like that:
const label = useMemo(() => {
if (!permissions) {
return 'No permissions available yet';
}
if (permissions === null || permissions?.download.public) {
return 'Accessible by everyone';
}
if (permissions.download?.authorized_ids.length) {
return `Accessible by ${permissions?.download.authorized_ids.join()}`;
}
return 'Accessible by owner only';
}, [permissions]);
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 think the label calculation isn't heavy enough to justify the use of useMemo :)
</Table> | ||
</TableContainer> | ||
{taskLoading || !task ? ( | ||
<Skeleton height="50px" width="457px"></Skeleton> |
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.
Maybe an oversight, can't it be a self closing tag?
<Skeleton height="50px" width="457px" />
Ready to be merged. Waiting for end of merge freeze. Thanks @Milouu ! |
Signed-off-by: Milouu <[email protected]>
Signed-off-by: Milouu <[email protected]>
Signed-off-by: Milouu <[email protected]>
Signed-off-by: Milouu <[email protected]>
Signed-off-by: Milouu <[email protected]>
… set Signed-off-by: Milouu <[email protected]>
Signed-off-by: Milouu <[email protected]>
17bf18b
to
6ff1c85
Compare
/e2e --tests=sdk,frontend |
End to end tests: ✔️ SUCCESS Awesome! 🎉 |
Description
Closes FL-1175
Screenshots