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 a dialog for skipping the analysis stage #3528

Merged
merged 8 commits into from
Apr 21, 2022
Merged

Add a dialog for skipping the analysis stage #3528

merged 8 commits into from
Apr 21, 2022

Conversation

knanao
Copy link
Member

@knanao knanao commented Apr 13, 2022

What this PR does / why we need it:
This enables it to skip just the analysis stage from web UI.
This means that this doesn't support the other stages.

skip-demo.mov

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Add a dialog for skipping the analysis stage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.69%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 77.54%. This pull request decreases coverage by -0.19%.

File Base Head Diff
src/api/deployments.ts 51.28% 48.89% -2.39%
src/components/deployments-detail-page/pipeline/index.tsx 88.46% 82.98% -5.48%
src/constants/metadata-keys.ts 100.00% 100.00% +0.00%
src/modules/deployments/index.ts 73.79% 72.64% -1.14%

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 77.54%. This pull request decreases coverage by -0.19%.

File Base Head Diff
src/api/deployments.ts 51.28% 48.89% -2.39%
src/components/deployments-detail-page/pipeline/index.tsx 88.46% 82.98% -5.48%
src/constants/metadata-keys.ts 100.00% 100.00% +0.00%
src/modules/deployments/index.ts 73.79% 72.64% -1.14%

@knanao knanao marked this pull request as ready for review April 18, 2022 05:54
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 77.50%. This pull request decreases coverage by -0.23%.

File Base Head Diff
src/components/deployments-detail-page/pipeline/index.tsx 88.46% 81.25% -7.21%
src/constants/metadata-keys.ts 100.00% 100.00% +0.00%
src/modules/deployments/index.ts 73.79% 72.64% -1.14%
src/api/deployments.ts 51.28% 48.89% -2.39%

@nghialv
Copy link
Member

nghialv commented Apr 18, 2022

@knanao Great!

I want to confirm one thing, was the "SKIP" popup shown after clicking the "ANALYSIS" stage?
If that, will it override the action of clicking the stage to see the log viewer?

1 similar comment
@nghialv
Copy link
Member

nghialv commented Apr 18, 2022

@knanao Great!

I want to confirm one thing, was the "SKIP" popup shown after clicking the "ANALYSIS" stage?
If that, will it override the action of clicking the stage to see the log viewer?

@knanao
Copy link
Member Author

knanao commented Apr 18, 2022

@nghialv
Good point of view!

I want to confirm one thing, was the "SKIP" popup shown after clicking the "ANALYSIS" stage?

Exactly, but I've already fixed it to show the log and open the dialog when clicking the stage by 629d951.

demo.mov

@nghialv
Copy link
Member

nghialv commented Apr 18, 2022

@knanao Thanks for your explanation.

I see. Now it shows both log viewer and confirmation popup once we clicked that stage, right?
Tbh, I feel it a bit annoying when showing that popup every time even when I just want to see the log viewer and don't intend to skip the stage.
So I think we should find somewhere to put a SKIP button to reveal the popup.

WDYT?

@knanao
Copy link
Member Author

knanao commented Apr 18, 2022

@nghialv
Well, I got your point.
For example, we can think about putting the SKIP button like below.
But we should get stage.id to stop the specific stage, that's why I feel like it's a little difficult.
Screen Shot 2022-04-18 at 17 10 40

@nghialv
Copy link
Member

nghialv commented Apr 19, 2022

@knanao Yes, I see. I am thinking of any way to add a stage action button (such as "SKIP", "APPROVE") in the stage area.
For example, add a small icon on the left half of the "ANALYSIS" stage button. 🤔

@nghialv
Copy link
Member

nghialv commented Apr 19, 2022

Or adding the SKIP, APPROVE button in the log viewer bar?

@khanhtc1202 Do you have any idea?

@nghialv
Copy link
Member

nghialv commented Apr 19, 2022

Another idea is changing the ANALYSIS stage to a button that holds a triangle on the right side to select an action. (Like the CANCEL button)

@khanhtc1202
Copy link
Member

The ANALYSIS stage button is quite small compared to others, so how about having text Skip clickable at the right bottom of that stage button 🤔

@khanhtc1202
Copy link
Member

khanhtc1202 commented Apr 19, 2022

Screen Shot 2022-04-19 at 9 49 20

Or maybe we can add an icon skip to the head of the log header line or text/button skip after stage description at that line. IMO, that skip feature should rarely be used or only be used in intention, which means not everyone wants to know about that at the first look. Making docs or letting users find out about that after time of use is a better choice. Besides, if we want to add the skip feature for other stages than ANALYSIS, changing the stage button is not a good idea, because every button would look so heavy with that kind of dropdown icon or text skip in it.

The first click to stage button should remain log view open action, we only add the skippable ability to the stage via an entry point which is added somewhere else, is my point.
wdyt?

@knanao
Copy link
Member Author

knanao commented Apr 19, 2022

@nghialv @khanhtc1202
Thank you guys for the good suggestions.

we can add an icon skip to the head of the log header line or text/button skip after stage description at that line.

I would think this is a good idea and I would like to adapt this one like this.
(this is just the demo view, and then actually this icon is displayed when the stage is just only running.)

What do you guys think about this?

Screen Shot 2022-04-19 at 13 11 24

@nghialv
Copy link
Member

nghialv commented Apr 21, 2022

Nice work!
/lgtm

@pipecd-bot pipecd-bot removed the lgtm label Apr 21, 2022
@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@khanhtc1202
Copy link
Member

Nice ✨ ✨
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@khanhtc1202
Copy link
Member

@knanao web test is failed 👀

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 35.56%. This pull request does not change code coverage.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 77.50%. This pull request decreases coverage by -0.23%.

File Base Head Diff
src/api/deployments.ts 51.28% 48.89% -2.39%
src/components/deployments-detail-page/log-viewer/index.tsx 85.11% 76.27% -8.84%
src/components/deployments-detail-page/pipeline/index.tsx 88.46% 88.10% -0.37%
src/constants/metadata-keys.ts 100.00% 100.00% +0.00%
src/modules/commands/index.ts 90.62% 93.75% +3.12%
src/modules/deployments/index.ts 73.79% 73.04% -0.74%
src/modules/project/index.ts 60.38% 56.60% -3.77%

@khanhtc1202
Copy link
Member

/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants