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

Indicate a build is in progress in Dashboard build page #2562

Closed
wants to merge 2 commits into from

Conversation

Yoda-Canada
Copy link
Contributor

@Yoda-Canada Yoda-Canada commented Dec 3, 2021

Fixes #2542

  • An animated indication shows a build is happening.
  • The animated indicator shows the percentage being built.
  • The indicator shows how long it's been going for.
  • When it's done, the yellow animated circle becomes a green checkmark or a red ' X'.

Issue This PR Addresses

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Dec 3, 2021

@humphd
Copy link
Contributor

humphd commented Dec 3, 2021

Post a short video of how it looks for reviewers, please.

@Yoda-Canada
Copy link
Contributor Author

@humphd
Copy link
Contributor

humphd commented Dec 3, 2021

NOTE: you can drag-and-drop video files into this Issue in a comment, and GitHub will host it for you.

We can't use a % since we don't have any concept of where we are in a build.

@Yoda-Canada
Copy link
Contributor Author

issue_2542.mp4

@Yoda-Canada
Copy link
Contributor Author

@humphd Do we have the concept of the time or just our estimate
a388a1182d01aed861346941cf7ceb6

@humphd
Copy link
Contributor

humphd commented Dec 3, 2021

We know when a build started (Datetime) and therefore can calculate how long it's been running, so we could do something like 1m 15s, yes.

@HyperTHD
Copy link
Contributor

HyperTHD commented Dec 4, 2021

You need to link the issue you're closing so that github automatically closes it when your PR gets merged.
Please do this ASAP @Yoda-Canada

@Yoda-Canada
Copy link
Contributor Author

Yoda-Canada commented Dec 8, 2021

Add the datetime calculator function. However it doesn't automatically triggered by build. I will try to do it tomorrow.

calculator.mp4.mp4

Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

Can you update the branch? Your changes to build.html should go into builds.hbc

Comment on lines 6 to 41
.box {
position: relative;
height: 50px;
width: 250px;
background: transparent;
border-radius: 5px;
}
.box svg {
position: absolute;
height: 160px;
width: 160px;
left: 25%;
top: 50%;
transform: translate(-50%, -50%) rotate(-90deg);
}
.box svg circle {
fill: transparent;
stroke: rgb(252, 206, 3);
stroke-width: 3;
stroke-dasharray: 3;
stroke-dashoffset: 0;
transform-origin: center;
animation: rotate 1s linear infinite, stroke 3.9s linear infinite;
}

.box .counter {
position: absolute;
top: 60%;
left: 25%;
transform: translate(-50%, -50%);
width: 70%;
height: 30px;
color: #e6b800;
text-align: center;
overflow: hidden;
}
.box .counter span {
font-size: 10px;
line-height: 25px;
font-weight: bold;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an icon from google material icons. Don't have to make your own.

Comment on lines 47 to 50
h8 {
color: #000;
position: absolute;
width: 100%;
text-align: right;
top: 25%;
right: 2%;
letter-spacing: 1px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this HTML tag doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it set the 'Staging' position.
1639161277(1)

Copy link
Contributor

Choose a reason for hiding this comment

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

What is h8 tag? Can you provide a link to its documentation?

src/api/status/public/css/animated-indication.css Outdated Show resolved Hide resolved

document.getElementById(
'calculatorData'
).innerHTML = `${days} days, ${hours} h ${minutes} m ${seconds} s`;
Copy link
Contributor

@Andrewnt219 Andrewnt219 Dec 10, 2021

Choose a reason for hiding this comment

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

A build is several hours at most, skip the day. Additionally, can you make it so that the units are only shown when it is > 0? For example, 3s instead of 0h 0m 3s. 1h 0m 0s should also be valid.

src/web/src/components/Posts/GitHubInfo.tsx Outdated Show resolved Hide resolved
src/web/src/student-quotes.js Outdated Show resolved Hide resolved
@Yoda-Canada
Copy link
Contributor Author

Can you update the branch? Your changes to build.html should go into builds.hbc
DO you mean I need to merge master? Is it correct to use the following command?

git pull upstream master
git checkout issue-2542
git merge master

@Andrewnt219
Copy link
Contributor

@Yoda-Canada

git checkout your-branch-name
git pull --rebase upstream master

Then solve all the conflict during the rebase.

@AmasiaNalbandian AmasiaNalbandian linked an issue Dec 10, 2021 that may be closed by this pull request
@AmasiaNalbandian
Copy link
Contributor

AmasiaNalbandian commented Dec 10, 2021

You need to link the issue you're closing so that github automatically closes it when your PR gets merged. Please do this ASAP @Yoda-Canada

I added the issue in the side manually, sorry about this. To correctly link an issue, please use one of the keywords from GitHub to link the issue to the PR. Can you please adjust the PR for this?

@Yoda-Canada
Copy link
Contributor Author

Can you update the branch? Your changes to build.html should go into builds.hbc
@Andrewnt219 when I change build.html into builds.hbc, webpage can't display the data time calculator. Could you help me to find the issue:
step 1: in the src/api/status/public/js/pages/build.js file add the following code

           import datetimeCalculator from '../build-log/datetime-calculator.js';
                window.addEventListener('load', () => {
                datetimeCalculator();
               setInterval(datetimeCalculator, 1000);
              }

step 2: in the src/api/status/src/views/builds.hbs file add the follwing code

        <div id="calculatorData" style="float: right"></div>

@Andrewnt219
Copy link
Contributor

Andrewnt219 commented Dec 14, 2021

Can you update your branch first?

when I change build.html into builds.hbc, webpage can't display the data time calculator. Could you help me to find the issue

Any screenshot for errors or any error messages will be helpful.

EDIT: Sorry, I made a typo there, you should look for builds.hbs not builds.hbc.

@Yoda-Canada
Copy link
Contributor Author

@humphd I have almost finished this issue, but when I want to commit my code, it displays an error. Could you help me?
image
image

@Yoda-Canada
Copy link
Contributor Author

image

@Andrewnt219
Copy link
Contributor

Try rebasing your branch and run pnpm i again.

@Yoda-Canada
Copy link
Contributor Author

I followed the instruction of https://dev.to/menghif/transition-to-pnpm-4alf and when I execute the pnpm install, there is an error
image

@Andrewnt219
Copy link
Contributor

On Window, you should open Powershell and run this script first.

Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy Unrestricted

See: https://stackoverflow.com/questions/41117421/ps1-cannot-be-loaded-because-running-scripts-is-disabled-on-this-system

@Yoda-Canada
Copy link
Contributor Author

@Andrewnt219 can I delete the src/api/status/public/css/animated-indication.css directly in PR 2562? I have deleted this file in local, but it still displays in github

@Yoda-Canada Yoda-Canada marked this pull request as ready for review December 15, 2021 15:59
@Andrewnt219
Copy link
Contributor

You have to add the deleted file to the commit as well.

ESLint test failed. You should run pnpm lint and fix those errors locally. Can you also update the PR description with a new video of the feature, and add how to test it locally as well?

@Andrewnt219
Copy link
Contributor

@Andrewnt219 I can't find the already deleted file -src/api/status/public/css/animated-indication.css. Can I create a new one, then delete and commit?

Deleting from the PR is fine too I guess.

how to fix the End-to-EndN Tests error? It looks like the npm error, and I have deleted the npm before installed pnpm

Not sure why it failed either. Just fix ESLint and push again.

@Yoda-Canada
Copy link
Contributor Author

In the file src\api\status\public\js\build-log\datetime-calculator.js , add the following test data in Line7-11:

  data = {
    "startedAt": "2021-12-16T01:45:57.509Z",
    "stoppedAt": "null",
    "code": 0
  }

image

New Recording - 12/15/2021, 11:21:59 PM

@Yoda-Canada
Copy link
Contributor Author

In the file src\api\status\public\js\build-log\datetime-calculator.js , add the following test data in Line7-11:

  data = {
    "startedAt": "2021-12-16T01:45:57.509Z",
    "stoppedAt": "2021-12-16T01:50:59.509Z",
    "code": 1
  }

image

@Yoda-Canada
Copy link
Contributor Author

In the file src\api\status\public\js\build-log\datetime-calculator.js , add the following test data in Line7-11:

  data = {
    "startedAt": "2021-12-16T01:45:57.509Z",
    "stoppedAt": "2021-12-16T01:50:59.509Z",
    "code": 0
  }

image

Copy link
Contributor

@Andrewnt219 Andrewnt219 left a comment

Choose a reason for hiding this comment

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

Glad to see this is taking shape. There is still room for improvement.

src/api/status/public/js/pages/build.js Outdated Show resolved Hide resolved
@@ -31,6 +31,24 @@
</div>
</div>
</nav>
<div class="container-fluid py-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this into the build header? There are two columns for "result" and "total duration".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the beginning, I want to move this into the build header. But after I studied issue #2416, I think they are different functions, maybe it's better to separate these two items

Copy link
Contributor

Choose a reason for hiding this comment

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

Are they? Your result and total duration are for current build, and they're the same as those two columns in build-header. Data is from the same source, too.

src/api/status/src/views/builds.hbs Outdated Show resolved Hide resolved
src/api/status/public/js/build-log/datetime-calculator.js Outdated Show resolved Hide resolved
src/api/status/public/js/build-log/datetime-calculator.js Outdated Show resolved Hide resolved
src/api/status/public/js/build-log/datetime-calculator.js Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Dec 16, 2021

I was just watching a build on Vercel, and this is how they do it, with a spinner that flips to a check-mark or 'x', and a pretty-printed time value:

Screen Shot 2021-12-16 at 5 05 00 PM

Screen Shot 2021-12-16 at 5 04 17 PM

@Andrewnt219
Copy link
Contributor

@Yoda-Canada any updates on the current state of this PR?

@humphd humphd added this to the 2.5 Release milestone Jan 13, 2022
@Yoda-Canada
Copy link
Contributor Author

@Andrewnt219 I haven't updated the current state yet. I will review it again.

@tpmai22
Copy link
Contributor

tpmai22 commented Jan 15, 2022

@Yoda-Canada how is everything for this PR, would you need some help rebasing it ?

@Yoda-Canada
Copy link
Contributor Author

@tpmai22 Most of the issues has been solved, except for the following issue. Could you help me solve this problem? I really appreciate your assistance.
image

@tpmai22
Copy link
Contributor

tpmai22 commented Jan 15, 2022

I would love to take a look and see

@Andrewnt219
Copy link
Contributor

@tpmai22 you may want to wait for #2653 to merge.

@tpmai22
Copy link
Contributor

tpmai22 commented Jan 15, 2022

@Andrewnt219 for this issue I try to catch up with the conversation between you and @Yoda-Canada but I'm still a bit lost. Would you mind to shred some light ?

@Andrewnt219
Copy link
Contributor

@tpmai22 in #2582, a build header was added to show the info of the ongoing build (see screenshots). At the moment, the header just shows the result "Error" or "Good". It is missing an "In progress" state.

The current PR is also showing animation/loading indicators of the 3 statuses in addition to just plain text.

@TDDR TDDR modified the milestones: 2.5 Release, 2.6 Release Jan 20, 2022
@tpmai22 tpmai22 mentioned this pull request Jan 26, 2022
8 tasks
@tpmai22
Copy link
Contributor

tpmai22 commented Jan 27, 2022

We can close this now refer to #2665

@tpmai22 tpmai22 closed this Jan 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.

Indicate a build is in progress in Dashboard build page
7 participants