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

feat: Add version information panel (#4312) #4376

Merged
merged 5 commits into from
Oct 6, 2020

Conversation

tetchel
Copy link
Contributor

@tetchel tetchel commented Sep 18, 2020

  • Add new extended version info panel that pops up when the version is clicked in the navbar
  • Refactor kustomize and helm versions to be more user-friendly (also affects argo CLI)

Signed-off-by: Tim Etchells tetchell@redhat.com

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • Maybe
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I've signed the CLA and my build is green (troubleshooting builds).

@tetchel tetchel marked this pull request as draft September 18, 2020 19:23
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2020

Codecov Report

Merging #4376 into master will increase coverage by 0.08%.
The diff coverage is 47.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
+ Coverage   41.17%   41.26%   +0.08%     
==========================================
  Files         122      122              
  Lines       16588    16603      +15     
==========================================
+ Hits         6830     6851      +21     
+ Misses       8776     8762      -14     
- Partials      982      990       +8     
Impacted Files Coverage Δ
util/kustomize/kustomize.go 62.28% <42.85%> (-3.74%) ⬇️
util/helm/helm.go 61.81% <60.00%> (-0.93%) ⬇️
util/health/health.go 72.00% <0.00%> (-8.00%) ⬇️
controller/appcontroller.go 50.17% <0.00%> (+2.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91e62bf...5f93f97. Read the comment docs.

@tetchel
Copy link
Contributor Author

tetchel commented Sep 18, 2020

new argocd version server output

argocd-server: v99.99.99+unknown
  BuildDate: 1970-01-01T00:00:00Z
  GitCommit:
  GitTreeState:
  GoVersion: go1.14.1
  Compiler: gc
  Platform: linux/amd64
  Ksonnet Version: v0.13.1
  Kustomize Version: v3.8.1 2020-07-16T00:58:46Z
  Helm Version: v3.3.1+g249e521
  Kubectl Version: v1.17.8

@tetchel
Copy link
Contributor Author

tetchel commented Sep 18, 2020

demo:
version-panel(1)

version-panel.mov.zip

@tetchel tetchel marked this pull request as ready for review September 18, 2020 19:56
@tetchel
Copy link
Contributor Author

tetchel commented Sep 18, 2020

resolves #4312

@tetchel
Copy link
Contributor Author

tetchel commented Sep 18, 2020

I am totally new to Go, and fairly new to React. so feel free to nitpick ;)

- Add new extended version info panel that pops up when the version is clicked in the navbar
- Refactor kustomize and helm versions to be more user-friendly (also affects argo CLI)

Signed-off-by: Tim Etchells <tetchell@redhat.com>
Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

Remove the copy-to-clipboard library, and it LGTM.

ui/src/app/version-info/components/version-info-panel.tsx Outdated Show resolved Hide resolved
ui/src/app/version-info/components/version-info.scss Outdated Show resolved Hide resolved
ui/src/app/version-info/components/version-info-panel.tsx Outdated Show resolved Hide resolved
ui/src/app/version-info/components/version-info-panel.tsx Outdated Show resolved Hide resolved
ui/src/app/version-info/components/version-info-panel.tsx Outdated Show resolved Hide resolved
ui/src/app/version-info/components/version-info.scss Outdated Show resolved Hide resolved
Improve copy button behaviour
Clean up comments

Signed-off-by: Tim Etchells <tetchell@redhat.com>
@rbreeze
Copy link
Member

rbreeze commented Oct 1, 2020

Love the new panel. I found that after removing the CSS it still looks almost identical so I think it's best to leave it out if possible. I also inlined VersionInfoButton to keep down the amount of new code added, and I think the VersionInfoPanel file should live in the shared directory.

LGTM, this is really useful.

@tetchel
Copy link
Contributor Author

tetchel commented Oct 2, 2020

I can appreciate the desire to keep the code as simple as possible, but I wouldn't have added the styles if I didn't think those details mattered.

Especially the styles on the version-info-button are important to allow a user to discover this panel; otherwise on mouse-over it looks like the tooltip is all you get. The :hover pointer and underline are what make it look clickable.

The styles on the panel itself are definitely more subjective and I am OK without most of those, however I think the Copy button is a little cramped now and the icon could use some space on the right.

image

The reason I set the min-width on the button was because the text changes when you click it, and I did not like the button changing widths on-click and then resetting.

@rbreeze
Copy link
Member

rbreeze commented Oct 6, 2020

I agree with everything you pointed out. I added back the styles to keep the copy button a consistent width, but inline because I don't think it warrants an additional file. I also added an additional &nbsp; to give it some extra space (the equivalent of margin-right: 2ch).

For the version button itself, I don't think there's a good way to add text-decoration: underline on hover without making the version button a new component, but I really don't think this should be its own component because it is so small. By changing it to <a> rather than <span> we get the cursor: pointer for free (it follows accessibility guidelines this way too), and I think this combined with the tooltip is enough for users to discover it. I also looked around elsewhere in the UI to see if we could "steal" a style that underlines on hover, but there isn't anywhere that uses that design pattern so I additionally think for consistency's sake, the underline is unnecessary.

If you agree, I think it's ready to merge. Thanks for working on this!

@tetchel
Copy link
Contributor Author

tetchel commented Oct 6, 2020

Personally I prefer new files for separate components even if they are simple, and [s]css files over inline styles and nbsp.

But, that is subjective enough that I am happy to merge this.

@rbreeze
Copy link
Member

rbreeze commented Oct 6, 2020

I often agree, but in my experience with Argo the standard seems to be a preference toward minimal amount of files/components. Glad you're okay with the changes. Thanks!

@rbreeze rbreeze merged commit 5592150 into argoproj:master Oct 6, 2020
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.

None yet

3 participants