-
Notifications
You must be signed in to change notification settings - Fork 159
feat: dark mode logo #8488
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: dark mode logo #8488
Conversation
| var dark bool | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "upload-logo [<org-name> [<path-to-image>]]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the brackets should stay - for the org, it defaults to your default org, and for the path it's not required for --remove. Just for correctness – I realize this isn't super user friendly, but I guess it's kind of a rare command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if you want to improve this, maybe we should consider rill org upload-logo [--light PATH] [--dark PATH] [--remove] where --light and --dark can optionally be sent at the same time, and --remove removes both the light and dark logo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably keep them separate, not sure if anyone has workflows on the old command.
Ye the syntax gets a bit strange, I'll add back brackets
cli/cmd/org/upload_logo.go
Outdated
| Use: "upload-logo <org-name> <path-to-image>", | ||
| Args: cobra.MaximumNArgs(2), | ||
| Short: "Upload a custom logo", | ||
| Short: "Upload a custom logo (use --dark for dark-mode variant; omit path only with --remove)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Short field should ideally be very short – consider moving this to Long: or add these descriptions to the individual flag descriptions
97b13bd to
4fcb2e4
Compare
begelundmuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* feat: dark mode logo * brackets
Adds the ability to upload dark mode logo variants
Checklist: