johman10 / flood-for-transmission

A Flood (https://github.com/Flood-UI/flood) clone for Transmission
GNU General Public License v3.0
353 stars 33 forks source link

Add support for column width in config.json #563

Closed meehien closed 7 months ago

meehien commented 8 months ago

Would it be possible to add support for column widths in config.json. Something along the lines of (but not necessarily thist):

{ .... "COLUMNS": [ "Name [400px]", "Progress [600px]", .... ] .... }

Thanks

defkev commented 8 months ago

This should be trivial to add by turning COLUMNS into an object instead of an array:

{
"COLUMNS": {
      "Name": 400,
      "Progress": 600,
   }
}

imho this should preserve the insertion order for string keys as of ES2015, if i am to believe [1] Stack Overflow: [1] https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order/38218582#38218582

defkev commented 8 months ago

Alright after some digging around i came up with a solution satisfying both cases (array and object) for backwards compatibility with existing config.json, you can even use 0 width to use the (build-in) defaults to enable a column without defining its width, e.g.:

{
   "COLUMNS": {
      "Name": 0,
      "Progress": 600,
      "Added": 0,
      "ETA": 123
   }
}
meehien commented 7 months ago

thanks @defkev this is exactly what I had in mind. I would like to suggest a few changes to your patch:

defkev commented 7 months ago

What do you mean

does not work with Firefox

???

I use FF myself (stable channel) which supports Array.includes() since v43: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes#browser_compatibility

The comment is indeed nonsense and should be an object, i'll update this tomorrow, thanks.

Cheers

Edit: Just test this in Nightly (126.0a1) and its working as expected. Post whatever error you are getting, if any.

meehien commented 7 months ago

I just re-tested now and seems to be working fine with the includes statement. Strange... Yesterday, when I tried running it, I was getting x.columns.includes is not a function, but it would seem that the error came from elsewhere. Thanks again for the patch.

johman10 commented 7 months ago

@meehien thanks a lot for your feature request. @defkev did a good job on setting up a PR for the required changes but I made some changes to the configuration to allow for potential future values to be added.

If you have time would you mind testing the PR #567? Would love to hear your feedback if you have any.

johman10 commented 7 months ago

@meehien I just pushed a fresh version addressing this issue. Please have a look at the README, pull the latest changes and test it out. Don't hesitate to let me know if you experience any issues.

Thanks a lot for your suggestion and I hope the changes suit your needs!

meehien commented 7 months ago

Hi @johman10. Thanks including this functionality in the main build. I also like the possibility of expanding on the customizations available. The new release works fine for me. Also, thanks again @defkev for taking this onboard. I was using your version up until today.