-
Notifications
You must be signed in to change notification settings - Fork 552
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: users can view the associated process for a task #18460
base: main
Are you sure you want to change the base?
Conversation
Tasklist Test Results143 files 143 suites 1h 37m 28s ⏱️ Results for commit cf343d8. ♻️ This comment has been updated with latest results. |
if (error !== null && error.variant === 'failed-response') { | ||
const {status} = error.response; | ||
if ( | ||
status === HTTP_STATUS_FORBIDDEN || | ||
status === HTTP_STATUS_NOT_FOUND | ||
) { | ||
return 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.
Why do we need this special case? I think we should just throw the error like we do everywhere
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 behaviour for a 404 Not Found error is different to a 500 Internal Server Error. I think we should explicitly handle business cases (not found and forbidden) since it makes it simpler to whoever consumes this hook.
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 behaviour for a 404 Not Found error is different to a 500 Internal Server Error. I think we should explicitly handle business cases (not found and forbidden) since it makes it simpler to whoever consumes this hook.
but this is basically goes against react-query conventions of throwing errors and it makes harder for the hook to be reused if want to have a different behavior. It would be impossible to build a 404 page for this for example.
I believe this logic should exist in the view layer
<Tag className={styles.version}>Version: {version}</Tag> | ||
</div> | ||
{bpmnXml !== null ? ( | ||
<Layer level={1} className={styles.diagramFrame}> |
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.
Did you have to explicitly add it because it's was already at level 3?
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.
--cds-layer
only uses the current layer which is the same as the background at level 0. So I need to increase it to level 1 to get the grey background.
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.
Then the proper fix should be to wrap somewhere with <Layer />
component
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.
@douglasbouttell-camunda This was left unanswered
@@ -10,3 +10,8 @@ | |||
border-top: 1px solid var(--cds-border-subtle); | |||
border-bottom: 1px solid var(--cds-border-subtle); | |||
} | |||
|
|||
.hidden { | |||
display: none !important; |
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.
Let's remove the important please. We can use CSS specificity to achieve the same
return response.json(); | ||
} | ||
|
||
throw error ?? new Error('Failed to fetch process instances'); |
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.
error
is never null
when response
is null
throw error ?? new Error('Failed to fetch process instances'); | |
throw error; |
if (error !== null && error.variant === 'failed-response') { | ||
const {status} = error.response; | ||
if ( | ||
status === HTTP_STATUS_FORBIDDEN || | ||
status === HTTP_STATUS_NOT_FOUND | ||
) { | ||
return 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.
The behaviour for a 404 Not Found error is different to a 500 Internal Server Error. I think we should explicitly handle business cases (not found and forbidden) since it makes it simpler to whoever consumes this hook.
but this is basically goes against react-query conventions of throwing errors and it makes harder for the hook to be reused if want to have a different behavior. It would be impossible to build a 404 page for this for example.
I believe this logic should exist in the view layer
@douglasbouttell-camunda Could you also rebase this branch? |
@douglasbouttell-camunda Could you fix this tooltip issue I mentioned somedays ago on this PR too? There's also an issue with the labels appearing on new vars, which is the wrong behavior. This must have come from a Carbon update, it should be a onliner too: |
These have been fixed in the latest commit and I added some visual regression snapshots so we catch this 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.
@douglasbouttell-camunda Just some small comments that were left, but it seems like there are some issues with the E2E tests that need to be attended too
<Tag className={styles.version}>Version: {version}</Tag> | ||
</div> | ||
{bpmnXml !== null ? ( | ||
<Layer level={1} className={styles.diagramFrame}> |
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.
@douglasbouttell-camunda This was left unanswered
As we discussed in Slack, when a task doesn't have the form but only task variables, then the content with task variables should occupy the full width. |
bc241e8
to
524d5b2
Compare
@esraagamal6 Can you please test this PR? |
9b70b12
to
c22e3f7
Compare
c22e3f7
to
cf343d8
Compare
Description
Allow users to view the associated process for a task. Highlight the activity which relates to the task.
This allows users to have increased visibility and context for a task.
Related issues
closes #17645 , #18706