homepage icon indicating copy to clipboard operation
homepage copied to clipboard

Feature: Add list view for custom api

Open sgrtye opened this issue 1 year ago • 18 comments

Proposed change

Closes #2872

Currently, the custom API widget is limited to displaying up to four label-value pairs horizontally, I am here proposing having a list view that extends vertically.

image

The added feature is 100% compatible with old services.yaml files. For the above example, the only difference in config between two widgets is that one has specified to have a list view.

I know the list view exceeds the 1 row 4 block widget guideline, but it is not unprecedented as it is directly copied from the Coin Market Cap widget, and the custom API widget is the perfect place to make exceptions for more customization.

Type of change

  • [ ] New service widget
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Documentation only
  • [ ] Other (please explain)

Checklist:

  • [x] If applicable, I have added corresponding documentation changes.
  • [x] If applicable, I have reviewed the feature and / or service widget guidelines.
  • [x] I have checked that all code style checks pass using pre-commit hooks and linting checks.
  • [x] If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

sgrtye avatar Feb 12 '24 15:02 sgrtye

Or even just display

shamoon avatar Feb 12 '24 16:02 shamoon

Thanks. Agree customapi is a reasonable place for this.

one quibble is you use “vertical view” and “list view” interchangeably which is confusing. Let’s just call it “list”. And instead of a Boolean let’s use something like displayType: list

I also find the currency and trend params potentially overly-specific (even your docs note the potential for confusion there). I need to look at that more closely, if they really belong here at all

@shamoon, thanks for your reply.

I will then have display of list and row which default to row.

Regarding the currency and trend parameters, since I am copying most of the code from the Coin Market Cap widget, I just kept them as they are, as they also fit my particular use case in mind. Also, I made them both optional, so it would not interfere with other usages.

Since I am using my own API, I can easily add currency symbols in my response, so I don't mind removing the currency parameter. Also, as specified in the documentation, it might be showing NaN when combined with data transformation if the value after transformation is not a number.

However, I do find the trend indicator very helpful; after all, it displays a line, and having a secondary value would be really helpful. I thought about having a second optional 'field', then it would be too tedious to have the value changing its color. So I just went with limiting the secondary field to a colored trend indicator.

Please share your thoughts on this, and I can change my code accordingly. Also, I appreciated that you agreed to have a list view for the custom API, looking forward to your reply.

sgrtye avatar Feb 12 '24 16:02 sgrtye

display: list and should default to block, I think.

Yea, the issue with currency and trend is I dont see them being used in any other case other than the market list you have above (are there any?). It's mixing specificity. If this PR is about adding a list view, it should be a pure list view, which has nothing to do with currency nor trending. They should both be removed from here.

shamoon avatar Feb 12 '24 16:02 shamoon

display: list and should default to block, I think.

Yea, the issue with currency and trend is I dont see them being used in any other case other than the market list you have above (are there any?). It's mixing specificity. If this PR is about adding a list view, it should be a pure list view, which has nothing to do with currency nor trending. They should both be removed from here.

Sounds good, I will update the code and docs.

However, can we have a secondary field for the list view, as there will be more space to display information? I can remove the coloring part and the percentage sign, just to keep an optional field as plain text for the potential use of additional information.

sgrtye avatar Feb 12 '24 17:02 sgrtye

However, can we have a secondary field for the list view, as there will be more space to display information? I can remove the coloring part and the percentage sign, just to keep an optional field as plain text for the potential use of additional information.

Yea that could work, it just needs to be generic (and shouldn't be named 'trend' presumably), like somehow you can specify it as a secondary field but it can be any of the types used by the customapi widget. LMK if that makes sense

shamoon avatar Feb 12 '24 17:02 shamoon

Yea that could work, it just needs to be generic (and shouldn't be named 'trend' presumably), like somehow you can specify it as a secondary field but it can be any of the types used by the customapi widget. LMK if that makes sense

Sounds good, I guess I will name it secondaryField or additionalField? I went through some docs for other widgets, this seems to be the naming convention used in this project.

I wonder if you find having another parameter to define the color of this information would be too much? It will be genetic and optional, and also it clarifies the boundary between the first field and the second field. I can have a list of options for colors to avoid any attack, and also add more customization to the widget.

So I am still trying the achieve a colored trend indicator more genetically.

sgrtye avatar Feb 12 '24 17:02 sgrtye

Either one, additionalField seems fine.

As for the color, that could be OK as its an optional parameter too (additionalFieldColor or the like) but definitely should be short and sweet for now. Perhaps supporting a hex color and something like "positiveNegative" (maybe theres a better name for that) which would be a special option to color red / green below / above zero.

shamoon avatar Feb 12 '24 17:02 shamoon

As for the color, that could be OK as its an optional parameter too (additionalFieldColor or the like) but definitely should be short and sweet for now. Perhaps supporting a hex color and something like "positiveNegative" (maybe theres a better name for that) which would be a special option to color red / green below / above zero.

Please correct me if I get it wrong, but a special option to color based on below/above zero(despite very much I would like to have), would first assume the additional field is not empty also it can compared to zero directly(with no trailing percentage sign).

In my option to keep the parameter simple and genetic, I can implement another function to be used when additionalField is present. So the color will be determined using the function that could interpret the additionalFieldColor parameter as face value or path to value in JSON. Also, it validates the value to be correct as a valid tailwind CSS text color(supporting tailwind colors, hex values, and/or opacity). In this way, the color could be static, or dynamically allocated specified by an API call. In this way, it eliminates the possibility of attack or misuse of the CSS class input.

There the end user can use static color code or specify a field in the API response that specifies the color code. So no assumption was made using the implementation and it is flexible to adapt to different use cases.

sgrtye avatar Feb 12 '24 18:02 sgrtye

Yes, additionalFieldColor would only be relevant if additionalField is specified, it wouldn't be a problem if not, though.

I didnt follow every point of the middle of your last comment, might just be easier to see what you mean. I agree, the simpler the config the better, absolutely. Again, with regards to the API providing the color, are there any other examples of that besides the stock market API youre using? I bet not. It seems too-specific...

The only thing to note is that tailwind classes need to be already included statically, e.g. https://github.com/gethomepage/homepage/blob/ebd384c62dcff0e0ec4b27097f4b9baee83a0bdb/tailwind.config.js so its not as simple as allowing any class (and IMHO its not worth it to add a bunch of classes there just for this). I'd be OK with being able to choose from the ones that are already included (which also includes green and red even though they're not on that list) instead of allowing any HEX. Again, let's keep it simple.

shamoon avatar Feb 12 '24 18:02 shamoon

I didnt follow every point of the middle of your last comment, might just be easier to see what you mean. I agree, the simpler the config the better, absolutely. Again, with regards to the API providing the color, are there any other examples of that besides the stock market API youre using? I bet not. It seems too-specific...

So firstly, no, there is no such API that provides this sort of color info, I run my own API server so supplying any sort of data does not bother me, this way it just proves more customization for those who want to control the color dynamically by API calls.

So I am thinking of just providing an additionalFieldColor that only accepts string color and auto which tries to infer the color by its value. I am thinking of using regular express to extract the first number in additionalField and compare it to zero. This way it is rather simple and clear, also one could try to rely on auto to change the color accordingly.

The only thing to note is that tailwind classes need to be already included statically, e.g. https://github.com/gethomepage/homepage/blob/ebd384c62dcff0e0ec4b27097f4b9baee83a0bdb/tailwind.config.js so its not as simple as allowing any class (and IMHO its not worth it to add a bunch of classes there just for this). I'd be OK with being able to choose from the ones that are already included (which also includes green and red even though they're not on that list) instead of allowing any HEX. Again, let's keep it simple.

As for the second part, I think tailwind CSS supports all the text colors that showing in it docs by default, I can only provide support for those colors which does not need any additional classes to be added. I will try to implement it later and see if that works.

sgrtye avatar Feb 12 '24 19:02 sgrtye

Sorry for the confusion, tailwind indeed purges all unused classes when creating the CSS file. Currently, the CSS file only includes about 20 text colors with various shades of color. presumedly is used by all different parts of the page.

There is no clear relation to what is included in the project already, I don't think there is an easy way to introduce a color selection without introducing more to the CSS file.

image

I am thinking of two solutions, there are 22 colors excluding white and black officially used by Tailwind, I can add 'text-color-300'(or another shade) to the CSS which only introduces 22 more classes which is relatively small when compared to the whole CSS file.

Or I am trying the find a way to reuse theme colors, as all of the colors are included in the CSS file already. However, I could not seem to be able to refer to the defined theme color for the text.

sgrtye avatar Feb 12 '24 22:02 sgrtye

I just figured out a way to reuse the theme color!!!! I can just instead of specifying a 'text-color-shade', I can do 'theme-color text-theme-shade'. Then I effectively added no extra CSS classes, but allowing a wide selection of colors.

By redefining the theme color at a local level, I can reuse the color defined in the CSS file to color the text. I think this might be the best solution for now, indeed it adds some coupling for the colors of the theme and the color available for the text, but I can't think of a better solution without refactoring the whole color system.

Do let me know what you think about the solution, and then I can implement the code.

sgrtye avatar Feb 12 '24 22:02 sgrtye

At this point I’m a bit lost in the weeds, I’d say just go ahead and do it, if you’re ok with that. Easier to talk about actual code than hypothetical

shamoon avatar Feb 12 '24 22:02 shamoon

At this point I’m a bit lost in the weeds, I’d say just go ahead and do it, if you’re ok with that. Easier to talk about actual code than hypothetical

I have done the coding part, I will have you updated after some testing.

sgrtye avatar Feb 12 '24 22:02 sgrtye

image

Here is an example of what I have implemented. There are four possibilities namely auto, black/white, color-shade, and some random value.

The auto option checks if there is a number in the value of additionalField, if so, it will display the color accordingly.

The black/white option will just display the text in black/white.

The color-shade option will also change the color if and only if the color is in the available list of colors.

Lastly, any other value, and by default, the additionalField will not have additional text color classes, so it will be displayed just like the Field.

Here is the list of available colors that were added by the theme coloring. image image

The only problem is that it has all the colors, but not all the shades. Going by 100s there is 300 and 400 missing in the current CSS. Currently, I just set shades 300 and 400 as invalid, but it might be better to include them as it will just be two more CSS classes and it completes the selection for coloring.

sgrtye avatar Feb 12 '24 23:02 sgrtye

I have the new implementation published, please let me know if there is anything else that you find needs changing. I will be checking the repository tomorrow. Thanks a lot for your feedback today, looking forward to contributing to the project soon.

sgrtye avatar Feb 13 '24 00:02 sgrtye

I have made changes that address your concerns, please take a look and let me know if there is still anything missing.

sgrtye avatar Feb 14 '24 20:02 sgrtye

  1. for auto, regex are slow, I think better to just try to parse as a number. I know you had it matching for any number in a string but I think most APIs will return a number alone when thats what its supposed to represent (and thus would be valuable for auto)

If you don't mind, I would like to add prefix to the data transformation, as it would also be valuable to have customization on both sides.

Also, there are some issues with the reserved characters, for example, % is not allowed. But supplying the suffix in quotation marks works to solve this issue. I will update the documentation to reflect the preferred way to input suffixes in the examples. image

sgrtye avatar Feb 14 '24 21:02 sgrtye

Great, thanks for making all the changes. I think this is solid.

Thank you. I'm eagerly anticipating the next release. I'd like to mention that the renaming of the mapping variable in the formatValue function was done because the function was called twice: once with mapping and once with mapping.additionalField. I felt that keeping it named mapping might be misleading, as it may not always refer to the mapping object.

sgrtye avatar Feb 15 '24 09:02 sgrtye

Trying this, I don't seem to be able to get the additionalField parameter to displey:

        widget:
            type: customapi
            url: http://10.0.10.9:9995/json?target=127.0.0.1:3493
            refreshInterval: 60000 # In milliseconds, set to ~60s
            method: GET
            display: list
            mappings:
                - field: 
                    ups: 
                      nutcase.battery.runtime
                  label: Runtime
                  additionalField: 
                    ups: 
                      input.voltage		
                - field: 
                    ups: nutcase.ups.load.watts
                  label: Power
                  additionalField: 
                    ups: 
                      nutcase.ups.status.composite

ArthurMitchell42 avatar Mar 15 '24 15:03 ArthurMitchell42

Commenting on merged PRs is not the best way to get help

shamoon avatar Mar 15 '24 15:03 shamoon