Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Add about window #131

Merged
merged 2 commits into from
Feb 7, 2022
Merged

Add about window #131

merged 2 commits into from
Feb 7, 2022

Conversation

evidolob
Copy link
Contributor

@evidolob evidolob commented Feb 3, 2022

This PR add new command About in trey context menu:
Screenshot 2022-02-03 at 12 29 37

The About window on mac:

Screenshot 2022-02-07 at 10 38 17

Also this PR add implemented with TypeScript, it brings tsconfig.json and some *.d.ts files with types.

Tray version taken from app.getVersion(), electron take it from package.json of app, so we need to update it during release

Also I not sure about bundle version, for now I use OpenShiftVersion value witch return by /version daemon call.

@@ -76,6 +76,7 @@ let configurationWindow = undefined;
let podmanWindow = undefined;
let setupWindow = undefined;
let pullsecretChangeWindow = undefined;
let aboutWindow = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

We also need to also destroy this window in the quitApp() function https://github.com/code-ready/tray-electron/blob/9cf6b7e91d59407a2c5878047133ea7ad0ef6d4b/main.js#L488

Without it am not able to exit the tray, and other exceptions are thrown when trying to do that

},

about: () => {
return ipcRenderer.invoke('get-about');
Copy link
Member

@anjannath anjannath Feb 4, 2022

Choose a reason for hiding this comment

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

👍🏽 nice, with invoke values can be returned, till now we were only using send and reply

@anjannath
Copy link
Member

anjannath commented Feb 4, 2022

Tested and working as expected, apart from #131 (comment) LGTM

Maybe squash all the commits into one?

@gbraad gbraad self-requested a review February 4, 2022 06:56
@gbraad
Copy link
Collaborator

gbraad commented Feb 4, 2022

Be aware that we also have a Podman version. However, that is dependent on the 'version' + 'preset' value. This is done in the MiniStatus windows for example.

https://github.com/code-ready/tray-electron/blob/9cf6b7e91d59407a2c5878047133ea7ad0ef6d4b/frontend/src/components/MiniStatusWindow.jsx#L19-L22

Discuss this with @praveenkumar and @anjannath maybe. Will also think about this some more. The problem is that this version is part of the bundle metadata. And we can't show both... perhaps not rely on it all and leave it in the MiniStatus as the only place?

This was linked to issues Feb 4, 2022
main.js Outdated
appVersion: app.getVersion(),
crcVersion: version.CrcVersion,
ocpBundleVersion: version.OpenshiftVersion
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the situation when the podman preset is chosen? In that case the version is not the OCP version.
That was the problem I referred to in: #125 (comment)

frontend/src/global.d.ts Show resolved Hide resolved
</div>
<div className={styles.clearBoth}>
<span className={styles.alignLeft}>Red Hat OpenShift Container Platform</span>
<span className={styles.alignRight}>{this.state.ocpBundleVersion}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also a podmanVersion.

@gbraad
Copy link
Collaborator

gbraad commented Feb 4, 2022

Maybe squash all the commits into one?

Also make sure to link the issue: Fixes #... in the first comment and git description.

main.js Show resolved Hide resolved
height: 450,
resizable: false,
show: false,
title: 'About',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, first to do #89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also need to remove <title> tag from index.html, as title will be ignored if <title> is set.

main.js Outdated Show resolved Hide resolved
frontend/package.json Outdated Show resolved Hide resolved

async componentDidMount(): Promise<void> {
// need to set title there, as index.html contains '<title>'
window.document.title = "About";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't the window creation not already set this?
We need to update all windows as part of #89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, see #131 (comment)

@@ -0,0 +1,29 @@
.alignLeft {
float: left;
font-weight: bold;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it almost feels in the images if the .alignRight also has a font-weight: bold assigned.
Can't check right now.

@evidolob evidolob requested a review from gbraad February 7, 2022 08:40
@evidolob
Copy link
Contributor Author

evidolob commented Feb 7, 2022

@gbraad @anjannath
I update this. Now it should show Podman version after crc-org/crc#2991

Also for CRC it will show commit in version {version}+{commit sha} form.

Could you look again on this?

Signed-off-by: Yevhen Vydolob <[email protected]>
}

private openDocumentation(): void {
window.api.openLinkInDefaultBrowser(`https://access.redhat.com/documentation/en-us/red_hat_codeready_containers/${this.state.crcVersion}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
this will make it easier to track with segment

@@ -51,15 +46,10 @@ export class AboutWindow extends React.Component {

<div className={styles.linksBox}>
<span className={styles.alignLeft}>
<Button variant="link" className={styles.docButton} icon={<ExternalLinkSquareAltIcon />} iconPosition="right" component="a" onClick={this.openDocumentation} target="_blank">
<Button variant="link" className={styles.docButton} icon={<ExternalLinkSquareAltIcon />} iconPosition="right" component="a" onClick={this.openDocumentationLink} target="_blank">
Copy link
Member

Choose a reason for hiding this comment

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

now since there's only one, maybe center align it? don't how it'd look, we can do it later though

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 could look like:
Screenshot 2022-02-07 at 14 14 05

Copy link
Collaborator

Choose a reason for hiding this comment

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

center align looks weird, like it is a button to press

@gbraad gbraad merged commit 057a3ba into crc-org:master Feb 7, 2022
@evidolob evidolob deleted the add-about-window branch February 7, 2022 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement About window Missing version information
3 participants