-
Notifications
You must be signed in to change notification settings - Fork 218
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 ability to set default theme for UI #2062
feat: Add ability to set default theme for UI #2062
Conversation
Thanks @Jamess-Lucass !! Works great if user doesn't already have theme set (which is what we want)
|
58413b7
to
c6ad607
Compare
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.
This PR is looking great @Jamess-Lucass ! Just incase it isn't clear, we have a couple schema's around our configuration file which need to be valid. That is whats failing during CI.
--- FAIL: Test_JSONSchema (0.01s)
schema_test.go:63:
Error Trace: /src/config/schema_test.go:63
Error: Should be true
Test: Test_JSONSchema
Messages: Schema is invalid
schema_test.go:65: ui: Additional property default_theme is not allowed
- JSON Schema: https://github.com/flipt-io/flipt/blob/main/config/flipt.schema.json
- CUE Schema: https://github.com/flipt-io/flipt/blob/main/config/flipt.schema.cue
These need to be updated to include your new string field.
Happy to give pointers if you're unfamiliar with either of these schema languages.
b461824
to
befab97
Compare
Ahh, thanks for pointing me in that direction, I've made those changes and run schema_test.go locally and they're passing. |
@all-contributors please add @Jamess-Lucass for code |
I've put up a pull request to add @Jamess-Lucass! 🎉 |
Looks great @Jamess-Lucass !! thank you for the contribution! |
Codecov Report
@@ Coverage Diff @@
## main #2062 +/- ##
=======================================
Coverage 70.41% 70.41%
=======================================
Files 70 70
Lines 6820 6822 +2
=======================================
+ Hits 4802 4804 +2
Misses 1741 1741
Partials 277 277
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Head branch was pushed to by a user without write access
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
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.
awesome! thanks again!
resolves issue: #1952.
As part of this PR I changed the initial theme values from
light
tosystem
which should better fit a default. eg.ui/src/app/preferences/preferencesSlice.ts#L15 and ui/src/app/preferences/Preferences.tsx#L47.
Please let me know if you want this reverted back to
light
.If this is accepted I can open a PR in flipt-io/docs to add this configuration value to https://www.flipt.io/docs/configuration/overview#general