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

ui: add attachment file icon in workflow #749

Merged
merged 3 commits into from
Feb 9, 2021
Merged

ui: add attachment file icon in workflow #749

merged 3 commits into from
Feb 9, 2021

Conversation

pheolink
Copy link

@pheolink pheolink commented Jan 21, 2021 •
edited by georgimld
Loading

Closes #234

@openkfwCI
Copy link

openkfwCI commented Jan 21, 2021 •
edited
Loading

NotesTime
✅Note for Reviewer: E2E tests on remote server succeededTue, 09 Feb 2021 10:46:17 +0000

Generated by E2E-Test

@pheolink pheolink marked this pull request as ready for review January 21, 2021 08:44
georgimld
georgimld requested changes Jan 21, 2021
View reviewed changes
Copy link
Collaborator

@georgimld georgimld left a comment

Choose a reason for hiding this comment

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

You only created tests where it is checked that the attachement icon is NOT displayed, but you should also write a test where the icon should be displayed (maybe in the documents_spec file)

@pheolink pheolink force-pushed the 234-workflow-attachfile-icon branch 2 times, most recently from 845beca to c0d20c8 Compare January 22, 2021 12:42
@Stezido Stezido force-pushed the 234-workflow-attachfile-icon branch from c0d20c8 to cae8522 Compare February 1, 2021 09:12
Stezido
Stezido previously requested changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@Stezido Stezido left a comment

Choose a reason for hiding this comment

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

Why same tests 2 times?
Remove it from the create/edit workflowitem specs since it's not matching with create/edit it's more a view thing.
I suggest to create a new spec named suproject_view_details.spec.
Also check e2e-tests they are failing

@pheolink pheolink force-pushed the 234-workflow-attachfile-icon branch 2 times, most recently from a7a3812 to b5c87ff Compare February 3, 2021 09:07
@pheolink pheolink requested a review from Stezido February 3, 2021 09:10
@pheolink pheolink force-pushed the 234-workflow-attachfile-icon branch 4 times, most recently from 9ec41e0 to 5a8b936 Compare February 3, 2021 09:31
@georgimld georgimld dismissed Stezido’s stale review February 3, 2021 11:02

changes were implemented

@georgimld georgimld force-pushed the 234-workflow-attachfile-icon branch 3 times, most recently from 1cb7839 to a4208a4 Compare February 3, 2021 13:11
@daniel-arnauer daniel-arnauer requested review from daniel-arnauer and removed request for Stezido February 9, 2021 10:03
@daniel-arnauer daniel-arnauer merged commit 1bc6832 into master Feb 9, 2021
@daniel-arnauer daniel-arnauer deleted the 234-workflow-attachfile-icon branch February 9, 2021 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgimld georgimld georgimld approved these changes

@Stezido Stezido Stezido left review comments

@daniel-arnauer daniel-arnauer Awaiting requested review from daniel-arnauer

Assignees

@pheolink pheolink

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ui: Move validation button for workflow items to workflow item table
5 participants
@pheolink @openkfwCI @Stezido @georgimld @daniel-arnauer