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

Added defaultsToFullScreen flag for Live/Details view,logs #2516

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

eiachh
Copy link
Contributor

@eiachh eiachh commented Jan 30, 2024

Added fullScreenlView flag to config.yaml to allow setting default behaviour for LiveView which includes yaml, helm history, describe, value_extender. 2506

I am not quite sure if this flag is the best under UI.

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@eiachh Nice!! Thank you Adam for this update!

README.md Show resolved Hide resolved
README.md Outdated
# By default all contexts wil use the dracula skin unless explicitly overridden in the context config file.
skin: dracula # => assumes the file skins/dracula.yaml is present in the $XDG_DATA_HOME/k9s/skins directory
# By default start all Live_View in full screen mode. (yaml, helm history, describe, value_extender) Default false
fullScreenLView: false
Copy link
Owner

Choose a reason for hiding this comment

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

Wondering if we could use a a better name for this??
Perhaps fullScreen or defaultsToFullScreen or activateFullScreen might be better choices??
Likely if users opt to default to FS likely the logs should operate as such. So I am thinking we should axe the log fullScreen and go with a single option to configure this ui bit in just one place??
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think having a defaultsToFullScreen option under Ui which sets all live/details view and logs is a better option.

For the naming I don't like fullScreenLView but fullScreen or defaultsToFullScreen in UI sounds like it is about the whole K9S not just the "popup" views. Since I could not come up with a better one I will just use defaultsToFullScreen.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the feedback Adam! Good point! I think that make sense.
Likely in the future we may end up breaking out sub-sections under UI for details view configs.

internal/view/live_view.go Outdated Show resolved Hide resolved
return nil
}

func (v *LiveView) setFullScreen(isFullScreen bool) {
Copy link
Owner

Choose a reason for hiding this comment

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

Likely we should refactor Live/Details view to offer this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added same functionality for Details view, if you could kindly check if thats what you meant by "refactor Live/Details view" that would be great.

Copy link
Owner

Choose a reason for hiding this comment

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

Right I was thinking combining the 2 to dry this up in a single place. This feels like a sep PR tho.

@eiachh
Copy link
Contributor Author

eiachh commented Feb 1, 2024

@derailed Thanks for this review as well, I believe I made the modifications.

@eiachh eiachh changed the title Added fullScreen flag for LiveView, updated readme.md Added defaultsToFullScreen flag for Live/Details view,logs Feb 1, 2024
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@eiachh Thank you for these updates Adam!

return nil
}

func (v *LiveView) setFullScreen(isFullScreen bool) {
Copy link
Owner

Choose a reason for hiding this comment

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

Right I was thinking combining the 2 to dry this up in a single place. This feels like a sep PR tho.

@derailed derailed merged commit 3c20f0d into derailed:master Feb 3, 2024
3 checks passed
@derailed derailed mentioned this pull request Feb 16, 2024
placintaalexandru pushed a commit to placintaalexandru/k9s that referenced this pull request Apr 3, 2024
…2516)

* Added fullScreen flag for LiveView, updated readme.md

* Added fullScreenLView default value for tests

* Extended DefaultsToFullScreen to include logView and DetailsView
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.

2 participants