Skip to content

Conversation

@Hosshii
Copy link
Member

@Hosshii Hosshii commented Mar 1, 2022

What this PR does / why we need it:
Display version information to web.

image

Which issue(s) this PR fixes:

Fixes #1958

Does this PR introduce a user-facing change?:

Show running version and documentation link on the web console

</Link>
</MenuItem>
<MenuItem disabled={true} dense={true}>
{process.env.STABLE_VERSION?.substring(0, 10)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{process.env.STABLE_VERSION?.substring(0, 10)}
Version: {process.env.STABLE_VERSION?.substring(0, 10)}

nits

Copy link
Member

Choose a reason for hiding this comment

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

btw, not sure why do we need to cut the string, why not just show the full version string? Since the last part is the commit hash and can be used to refer to source code on pipecd repo, which could be useful in some cases 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

image

I think this one is easier to understand, but the display looks like this. Which one do you think is better?

Copy link
Member

Choose a reason for hiding this comment

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

I get your point 👍 Then, how about remove the Version: part and show the full version string?

Copy link
Member Author

@Hosshii Hosshii Mar 1, 2022

Choose a reason for hiding this comment

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

image

Sorry, my comment was not updated and I missed it.

This is what it looks like when I put them all up. Personally, I find it too long.

Copy link
Member

Choose a reason for hiding this comment

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

That is exactly what I was thinking about.

Copy link
Member

Choose a reason for hiding this comment

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

About the version string, I think showing the full one is fine. Because on the production it is not that long.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!
I changed Docs to Documentation and add a icon.
image

Copy link
Member

Choose a reason for hiding this comment

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

Looks so beautiful!

Just a nit:

How do you think about moving the menu popup to be lower to avoid overlapping the tab bar?
We can resolve it in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Than sounds good to me.

@pipecd-bot
Copy link
Collaborator

COVERAGE

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

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 78.01%. This pull request decreases coverage by -0.05%.

File Base Head Diff
src/modules/project/index.ts 60.38% 56.60% -3.77%

@pipecd-bot
Copy link
Collaborator

COVERAGE

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

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for javascript is 78.09%. This pull request increases coverage by 0.03%.

File Base Head Diff
src/modules/commands/index.ts 90.62% 93.75% +3.12%

target="_blank"
rel="noreferrer"
>
Docs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Docs
Documentation

@nghialv
Copy link
Member

nghialv commented Mar 2, 2022

Great job! Thank you.
/lgtm

Nit about the release note

Show running version and documentation link on the web console

@pipecd-bot pipecd-bot added lgtm and removed lgtm labels Mar 2, 2022
@khanhtc1202
Copy link
Member

It's beautiful 😁
/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.

@Hosshii
Copy link
Member Author

Hosshii commented Mar 2, 2022

@nghialv
Sorry, I didn't push the code because I thought there might be more changes. I have pushed 986df95 that apply this comment #3328 (comment).

@nghialv
Copy link
Member

nghialv commented Mar 2, 2022

@nghialv Sorry, I didn't push the code because I thought there might be more changes. I have pushed 986df95 .

Got it. 👍

@pipecd-bot pipecd-bot merged commit 2b4b9d4 into master Mar 2, 2022
@pipecd-bot pipecd-bot deleted the web_version branch March 2, 2022 03:09
pipecd-bot pushed a commit that referenced this pull request Mar 2, 2022
**What this PR does / why we need it**:
Move the menu for visibility. And fix version placeholder.
before
<img width="377" alt="image" src="https://user-images.githubusercontent.com/49914427/156295069-38ecaa97-facb-4845-8e1b-87d0b745d359.png">

after
<img width="382" alt="image" src="https://user-images.githubusercontent.com/49914427/156294841-49850e17-463c-4b5a-9732-d57ebe8550ff.png">


**Which issue(s) this PR fixes**:

#3328 (comment)

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
-->
```release-note
Move menu to be lower to avoid overlapping the tab bar.
```

This PR was merged by Kapetanios.
pipecd-bot pushed a commit that referenced this pull request Mar 3, 2022
**What this PR does / why we need it**:
This PR changes [versions](#3328) to selectable and copyable for convenience.
<img width="376" alt="image" src="https://user-images.githubusercontent.com/49914427/156507069-2061276a-4dd6-428b-96d0-f25fc495a8dc.png">


**Which issue(s) this PR fixes**:

Fixes #

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
-->
```release-note
NONE
```

This PR was merged by Kapetanios.
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.

Display version information to the web frontend for easy reporting

6 participants