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

Add full-widget-sized job detail view #46

Closed
wants to merge 46 commits into from
Closed

Add full-widget-sized job detail view #46

wants to merge 46 commits into from

Conversation

andrii-i
Copy link
Collaborator

This PR adds full-widget-sized job details view as per Issue #5

Currently all fields of IDescribeJob (that extends ICreateJob) are rendered even if they are not filled. email_notifications field is not rendered

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch andrii-i/jupyter-scheduler/job-detail


export interface IJobDetailProps {
app: JupyterFrontEnd;
model: IJobDetailModel;
modelChanged: (model: IJobDetailModel) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing for you to do right now, but when #44 is merged in, there will be a conflict because this is being renamed to handleModelChange.

/>
<TextFieldStyled
label={trans.__('start_time')}
defaultValue={job?.start_time?.toString() ?? ''}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be converted from epoch seconds to a readable format (see the existing list view for more about how we do this)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to open another issue if this is too complex to do right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adapted your solution, otherwise would probably be too complex for now

}}
/>
<TextFieldStyled
label={trans.__('start_time')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field is already mentioned on line 305 above. Delete one occurrence of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed 305 occurence

}}
/>
<TextFieldStyled
label={trans.__('status_message')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
label={trans.__('status_message')}
label={trans.__('Status message')}

Shouldn't this be displayed right after "Status"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As it is now, status_message is always null in job object that server/API returns based on jobId while status contains actual status enum / string
Screen Shot 2022-09-26 at 1 56 15 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

status_message removed completely as it's not implemented

/>
<TextFieldStyled
label={trans.__('end_time')}
defaultValue={job?.end_time?.toString() ?? ''}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be converted from epoch seconds to a human-readable format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, adapted your solution

andrii-i and others added 19 commits September 26, 2022 13:52
@andrii-i
Copy link
Collaborator Author

All comments are solved / answered

@3coins 3coins mentioned this pull request Sep 27, 2022
@3coins 3coins closed this Sep 27, 2022
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