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.

Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
@divanshu-go divanshu-go requested a review from a team as a code owner October 22, 2024 18:43
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
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 <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
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 <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
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 <sabrinabryan1990@gmail.com>
@divanshu-go
Copy link
Contributor Author

@Tpuljak sorry the issue still need some work .
image

Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
Signed-off-by: bryans-go <sabrinabryan1990@gmail.com>
@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 <divanshugrover2009@gmail.com>
@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
Signed-off-by: Divanshu Grover <divanshugrover2009@gmail.com>
Signed-off-by: Divanshu Grover <divanshugrover2009@gmail.com>
@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 <divanshugrover2009@gmail.com>
Signed-off-by: Divanshu Grover <divanshugrover2009@gmail.com>
@divanshu-go
Copy link
Contributor Author

@idagelic Done !

Signed-off-by: Divanshu Grover <divanshugrover2009@gmail.com>
Signed-off-by: Divanshu Grover <divanshugrover2009@gmail.com>
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?

@Tpuljak
Copy link
Member

Tpuljak commented Nov 14, 2024

@divanshu-go any updates for this?

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