feat: make initial theme configurable in app-config#281
Conversation
|
Hi @EdithBirrer1, thanks for the contribution. Please open a corresponding issue first before creating a PR as per our contribution guidelines. Also, you need to sign the Eclipse Contributor Agreement before any PR can be merged. |
timdah
left a comment
timdah
left a comment
There was a problem hiding this comment.
Hey @EdithBirrer1, thanks for your contribution. It's a good idea in general, but please tweak your implementation and then we are good to go.
| if (appConfig.selectedTheme) { | ||
| this.stateService.setTheme(appConfig.selectedTheme); | ||
| } |
There was a problem hiding this comment.
Lets move this to the setAppConfig method in the state service and most important, this overrides the users theme selection stored in the local storage. So on application load the user selection is lost. The local storage theme config must have priority.
There was a problem hiding this comment.
Thanks for the input, will change it accordingly.
Co-authored-by: Tim Dahlmanns <13997715+timdah@users.noreply.github.com>
Co-authored-by: Tim Dahlmanns <13997715+timdah@users.noreply.github.com>
I'm very sorry. I had read the guidelines, but Legal Agreemend and all seemed very "heavy" for a 5 line change. But you are right, rules are rules. Now I hope I comply with them. Sorry again. |
ronjaquensel
changed the title
|
The check fails because I have mistakenly committed my changes with the e-mail address (as recommended by github) EdithBirrer1@users.noreply.github.com instead of my real e-mail address. How should I proceed to fix it? I hesitate to make an "amend" commit... |
There was a problem hiding this comment.
Move it one line up, because right now it is inbetween the lines for enableUserConfig. Also default should be undefinded and the linebreak after is missing. I'm reviewing on my phone, so I'm unable to suggest the changes 😄
Otherwise, LGTM.
Regarding the mail adress: Commit these changes with the correct adress and the check should pass.
There was a problem hiding this comment.
Misunderstood the "This user-specific configuration ..." to apply for the whole configuration, so that the whole app-config is stored locally.
Concerning the linebreak - there is one. (Or is it a Linux/Windows line ending issue?)
|
@EdithBirrer1 linebreaks seem to be fine, rendering looks correct. Regarding the ECA issue with the wrong email: you can revert the commits locally, re-commit the changes with the other mail address and then force-push the branch. That should solve it. |
|
@EdithBirrer1 any update? |
|
I tried to fix it: revert the commits I have done with the wrong e-mail-address and then did the changes with the correct e-mail-address, but this apparently did not help... |
Sure, fine with me. |
|
Superseded by #290 |
3 participants
What this PR changes/adds
The initially loaded theme can be set using the variable
selectedThemein app_config.json. (It is still possible to change to another theme manually.)Why it does that
If there are several dashboards (e.g. for several connectors), it is useful to distinguish them by color.
I know that one dashboard could display several connectors, but it can be an advantage to have several dashboard instances, one for each connector. E.g. in setups where the dashboard is limited by networking mechanisms so it can only access "its own connector". And if there are several dashboards, it is possible (and useful) to distinguish them via the theme, so it is instantly visible which dashboard shows which connector.
Who will sponsor this feature?
@ronjaquensel Could you please review? A very small change, but could be very useful in the described situations.
Linked Issue(s)
#283