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.

Sign up for GitHub

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

Jump to bottom

Add tasks details #61

Merged
merged 28 commits into from
Dec 15, 2018
Merged

Add tasks details #61

merged 28 commits into from
Dec 15, 2018

Conversation

proszewskip
Copy link
Owner

No description provided.

@proszewskip proszewskip self-assigned this Dec 13, 2018
@proszewskip proszewskip requested a review from Gelio December 13, 2018 20:21
Gelio
Gelio requested changes Dec 13, 2018
View reviewed changes
Copy link
Collaborator

@Gelio Gelio left a comment

Choose a reason for hiding this comment

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

The code looks good to me, with some exceptions 😄 Take a look at the comments

frontend/pages/distributed-tasks/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-nodes/table/table.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/types.ts Outdated Show resolved Hide resolved
frontend/src/features/subtasks/table/table/subtask-status-cell.tsx Outdated Show resolved Hide resolved
frontend/src/features/subtasks/table/table/subtask-status-filter.tsx Show resolved Hide resolved
frontend/src/features/subtasks/table/table/table.tsx Outdated Show resolved Hide resolved
frontend/src/product-specific/sidebar/authenticated-sidebar-menu-items.ts Outdated Show resolved Hide resolved
@proszewskip
Copy link
Owner Author

@Gelio I have updated this PR, please review again.

Gelio
Gelio requested changes Dec 14, 2018
View reviewed changes
frontend/src/features/distributed-task-definitions/details/details.tsx Show resolved Hide resolved
frontend/src/features/distributed-task-definitions/details/types.ts Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/format-distributed-task-status.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/get-details-grid.tsx Outdated Show resolved Hide resolved
frontend/src/features/subtasks/table/table/subtask-status-cell.tsx Outdated Show resolved Hide resolved
frontend/src/features/subtasks/table/table/table.tsx Show resolved Hide resolved
@Gelio
Copy link
Collaborator

Gelio commented Dec 15, 2018

Detale dla DistributedTask

@Gelio
Copy link
Collaborator

Gelio commented Dec 15, 2018

I noticed there is an underline on the Download buttons on the details page.

image

Let's try to remove it to achieve the same visual style as other buttons. My suggestions to fix this is to add a class in src/styles.css:

a.without-underline {
  text-decoration: none;
}

and apply it on the a tag:

<a href={inputDataUrl} download={true} className="without-underline">

This goes for both buttons.

Also, in the same place, let's add a Pane that wraps the a tag and apply the marginRight on it, instead of the button inside the a tag. When the margin is applied inside the button, when the element is focused via the keyboard, it looks weird:
image

Gelio
Gelio requested changes Dec 15, 2018
View reviewed changes
Copy link
Collaborator

@Gelio Gelio left a comment

Choose a reason for hiding this comment

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

Some more changes to go 😄

frontend/src/features/distributed-task-definitions/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-task-definitions/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
frontend/src/features/distributed-tasks/details/details.tsx Outdated Show resolved Hide resolved
Gelio
Gelio requested changes Dec 15, 2018
View reviewed changes
Server/Models/Subtask.cs Outdated
@@ -10,6 +10,7 @@ public class Subtask : Identifiable
[Attr("sequence-number", isImmutable: true)]
public int SequenceNumber { get; set; }

[Attr("distributed-task-id")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I reckon we should add isImmutable: true to this attribute

frontend/pages/distributed-tasks/details.tsx Outdated
class DetailsPage extends PureComponent<
DistributedTaskDetailsProps & WithRouterProps
> {
public static getInitialProps: GetInitialPropsFn = ({ query }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move the implementation of getInitialProps to inside src/features, similar to how other pages implement getInitialProps

Copy link
Collaborator

Choose a reason for hiding this comment

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

This way all the logic regarding a feature is in one directory instead of in two

frontend/src/features/distributed-tasks/details/details.tsx Outdated
</Pane>

<Pane marginRight={minorScale(2)} display="inline">
<a href={resultsUrl} download={true} className="without-underline">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's only pass the href when taskDone === true. Currently even though the button is disabled, the user may still click it and be redirected to an API endpoint for downloading the results.

<a href={taskDone ? resultsUrl : null} ...

@Gelio
Copy link
Collaborator

Gelio commented Dec 15, 2018

Welp, now there are merge conflicts 😢 Could you apply the fixes you made to table-example.tsx to where it is now moved, i.e. src\features\distributed-task-definitions\table\table.tsx? (remove a custom implementation of TextCell and import the one from components/data-table/cells/text-cell?

proszewskip added 17 commits December 15, 2018 21:47
[frontend] add distributed task model
5775877
[frontend] add distributed tasks details
447c5ad
[frontend] add subtask model
4648dab
[frontend] add subtasks table
5a7d65d
[frontend] add distributed task details page
5b2da70
[frontend] remove unused file
1a9080e
[frontend] add subtasks table to details page
aa5bab3
[frontend] fix wrong header name
acb1064
[frontend] use dictionary for status mapping
8655b92
[server] enable filtering by distributed task id
15000e7
[frontend] extract TextCell
9b840e3
[frontend] filter subtasks table by parent task id
ee57649
[frontend] add default option for subtask status filtering
fb325a4
[frontend] separate render into multiple functions
4f040da
[frontend] move button clicks handlers to details components
e9e2c2e
[frontend] fix typo
70092ca
[frontend] reduce scope of kitsu
aa20ae4
@Gelio Gelio merged commit 8f04fde into master Dec 15, 2018
@Gelio Gelio deleted the add-tasks-details branch December 15, 2018 20:57
proszewskip pushed a commit that referenced this pull request Feb 23, 2020
@Gelio
Merge pull request #61 from proszewskip/add-tasks-details
5ce4de0 
Add tasks details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gelio Gelio Gelio approved these changes

Assignees

@proszewskip proszewskip

Labels
frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@proszewskip @Gelio