Makes ng-gauge highlights width configurable #445
Conversation
jaredjensen
changed the title
|
Thanks for this PR. I feel bad for sharing this but it's worth it. In the next version (v3), gauges progress bars and gauge value change colors as they enter/leave zones. KIP will also have an all black theme with a set of 7 bright widget colors to help with sunlight visibility. I'll add an all white next which, even if I prefer black, look sharper in direct sunlight. Maybe you can take a look at the beta and see if this is eliminates the need to resize highlights? It's an unfinished beta product meant to test the new "gesture first" layout (no bottom navbar, swipe in four directions to obtain the same navbar fonctions. |
|
@godind I did see the comment about improving gauges in v3 in #410, and the improvements you described sound great! I started building a "green is good" style native linux app to achieve the same thing when I stumbled on your plugin. I'm happy to test v3, but I made this PR because we had an alternator fire on our last trip, and I'm feeling a bit urgent about getting better monitoring in place. I can run my fork until v3 is available if you don't want to merge it, though. |
|
I'll push it in. Can you also do the linear and steelgauge please? |
|
You bet, I'll try to knock those out tonight. |
|
Thanks! If you want to try the beta: To uninstall, use the SK AppStore UI uninstall and reinstall the latest. It's a work in progress so not close to final. Have a look at the gauge zones and colour. It should be more obvious now. Use swipe up/down to change dashboard and swipe left/right to open action and notification sidebars. Have fun. |
|
I pushed a couple more commits to add highlights width to the linear gauge and fix the initial load issue with the steel gauge. I don't think I can make the highlights width configurable on the steel gauge because it seems to be hard-coded, if I'm reading it right. |
|
Oh. True. Steelserie gauge color the existing bar. Sorry I forgot. I'll check the PR today or tomorrow as I have a few days to play with KIP. |
godind
changed the title
| @@ -168,7 +167,10 @@ | |||
| <mat-option value="capacity">Capacity</mat-option> | |||
| </mat-select> | |||
| </mat-form-field> | |||
|
|
|||
| <mat-form-field formGroupName="gauge" class="options-grid-span2"> | |||
There was a problem hiding this comment.
Would'nt we be better of with a single new @if statement rather then repeating the template code?
There was a problem hiding this comment.
I did that originally, but it seems better (to me) to keep all config for a given component together and accept the duplication. The way it's organized now makes it sorta hard to visual the final layout for a given component, and I can imagine cases where you may want a given setting to span different column widths for different components.
But this isn't a strong opinion, and I probably should have just stuck with the pattern you've got in place.
There was a problem hiding this comment.
The component uses formbuilder to dynamically parse the config object.
The template part is like 7 years old and ugly - ageed! The logic is just to run by the config or form object properties and values and output the approriate field controls. It's all fits in a 2 columns grid layout. It's not super flexible from a UX prespective, but with lots of Widgets and options, it needed something generic. You can use @if and @switch to adjust say, column span base on config properties, if necessary. I'm afraid that by repeating it this way, we will forget to check all config options and conditionnal cases. Months can go by until coding work and we tend to forget these things...plus we have no tests!
I'm all ears to see someone submit a PR with something better.
There was a problem hiding this comment.
Totally get it. I've been doing this 30+ years and have my own experience with maintaining "legacy" code, often of my own creation. =)
src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts
Outdated
Show resolved
Hide resolved
src/app/widgets/widget-gauge-ng-radial/widget-gauge-ng-radial.component.ts
Outdated
Show resolved
Hide resolved
| @@ -171,7 +173,7 @@ export class WidgetGaugeNgRadialComponent extends BaseWidgetComponent implements | |||
| this.gaugeOptions.valueDec = this.widgetProperties.config.numDecimal !== undefined && this.widgetProperties.config.numDecimal !== null ? this.widgetProperties.config.numDecimal : 2; | |||
| this.gaugeOptions.majorTicksInt = this.widgetProperties.config.numInt !== undefined && this.widgetProperties.config.numInt !== null ? this.widgetProperties.config.numInt : 1; | |||
| this.gaugeOptions.majorTicksDec = this.widgetProperties.config.numDecimal !== undefined && this.widgetProperties.config.numDecimal !== null ? this.widgetProperties.config.numDecimal : 2; | |||
| this.gaugeOptions.highlightsWidth = 0; | |||
| this.gaugeOptions.highlightsWidth = this.widgetProperties.config.gauge.highlightsWidth !== undefined && this.widgetProperties.config.gauge.highlightsWidth !== null ? this.widgetProperties.config.gauge.highlightsWidth : 5; | |||
There was a problem hiding this comment.
Reworking initWidget() will remove the need to test highlightsWidth for undefined or null.
There was a problem hiding this comment.
I think specifically adding highlightsWidth to defaultConfig makes this check unnecessary. I tested that by manually editing the local storage config to remove highlightsWidth and reloading the page, and the result was that the default width was used. This aligns with my review of validateConfig() and your comments about how the intent of that function is to maintain backward compatibility.
src/app/widgets/widget-gauge-steel/widget-gauge-steel.component.ts
Outdated
Show resolved
Hide resolved
src/app/widgets/widget-gauge-ng-linear/widget-gauge-ng-linear.component.ts
Outdated
Show resolved
Hide resolved
jaredjensen
force-pushed
the
gauge-highlights
branch
from
3d904a3 to
111fbcb
Compare
|
@jaredjensen merged. Thanks! |
This adds a "highlights width" property to the radial gauge config with a range of
0-25. It still defaults to5, but that value is too skinny for my old eyes.It also looks like there's a bug that prevents zones (and therefore highlights) from being available on initial load. I clumsily fixed this in the 2nd commit by moving
initWidget()afterobserveDataStream(), otherwise it seems the data service_pathRegisterisn't loaded when the gauge tries to subscribe to metadata. There's probably a better way to solve this, but it felt that would require a more fundamental change that would affect all widgets (although other widgets likely have this problem, too).