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

fix: add viewport for showing summary #1284

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

divanshu-go
Copy link
Contributor

@divanshu-go divanshu-go commented Oct 22, 2024

fix: add viewport for showing project / workspace summary

Description

this PR adds support for scrollable project summary by adding viewport

  • This change requires a documentation update
  • I have made corresponding changes to the documentation

Related Issue(s)

Closes #1283
Closes #1298

Screenshots

image

Screencast.from.2024-10-23.12-17-49.mp4

Notes

Please add any relevant notes if necessary.

@divanshu-go divanshu-go requested a review from a team as a code owner October 22, 2024 18:43
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
pkg/views/workspace/create/summary.go Outdated Show resolved Hide resolved
pkg/views/workspace/create/summary.go Outdated Show resolved Hide resolved
pkg/views/workspace/create/summary.go Show resolved Hide resolved
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

One minor comment to resolve. Nice work.

pkg/views/workspace/create/configuration.go Outdated Show resolved Hide resolved
Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
Tpuljak
Tpuljak previously approved these changes Oct 24, 2024
@Tpuljak Tpuljak requested a review from idagelic October 24, 2024 07:52
Signed-off-by: bryans-go <[email protected]>
@divanshu-go
Copy link
Contributor Author

@Tpuljak sorry the issue still need some work .
image

Signed-off-by: bryans-go <[email protected]>
Signed-off-by: bryans-go <[email protected]>
@divanshu-go
Copy link
Contributor Author

Done.

Screencast.from.2024-10-24.18-19-58.mp4

Tpuljak
Tpuljak previously approved these changes Oct 25, 2024
Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

image

image

Is it possible to have the "SUMMARY" title inside the box? The layout/paddings at least seem a bit off

Signed-off-by: Divanshu Grover <[email protected]>
@divanshu-go
Copy link
Contributor Author

@idagelic done !
image

Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

This looks good - left a minor suggestion

pkg/views/util/text_wrapper.go Outdated Show resolved Hide resolved
@divanshu-go
Copy link
Contributor Author

@idagelic also fixed #1298

Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

Yes, this seems to have fixed the env vars shuffling issue. Good work.

I have noticed some issues regarding displaying environment variables.
Here are a project-config add and a multiproject example:

Project config add - some env vars missing, scrolling does not do anything
image
image

Multi-project creation - sample-go project env vars are not shown
image
image

I see the env var display although better is not perfect on main either. Let me know if this is an easy fix as a part of this PR. Otherwise I will just open another issue for it

Signed-off-by: Divanshu Grover <[email protected]>
Signed-off-by: Divanshu Grover <[email protected]>
@divanshu-go
Copy link
Contributor Author

@idagelic Done !

Signed-off-by: Divanshu Grover <[email protected]>
Signed-off-by: Divanshu Grover <[email protected]>
Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

image

All the env vars seem to appear correctly now. However, the second issue (second set of screenshots) still exists - I added two env var entries for Project 2 but they aren't shown. Does the multi-project variant work on your end?

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.

Summary view env var weirdness non-scrollable TUI on multiple project summary view
3 participants