search

Novik / ruTorrent

Yet another web front-end for rTorrent
Other
2.03k stars 414 forks source link

Add responsive side panel #2709

Closed jevenski closed 2 months ago

jevenski commented 3 months ago
stickz commented 3 months ago

Could you review the white background on the responsive side panel for different themes and make adjustments where appropriate?

stickz commented 3 months ago

@TrimmingFool We are using Bootstrap not Tailwind. We need this wrapper for mobile purposes.

TrimmingFool commented 3 months ago

@stickz Ah, thanks! That explains it :smile: Apparently, I am out pretty of the loop.

I fear that, besides the clutter, dynamically toggling utility css-classes with jquery means that model, view and control can/will not really be separate.

I just wanted to raise this concern, as turning the category list into a web-component, had the intention of improving this separation. It seems a little late to pivot now but maybe this could be considered at another time.

@jevenski @stickz Anyway, thank you, for taking this on!

jevenski commented 3 months ago

I feel, that tailwind is better suited for styling individual components, where styles rarely need to be reused. I imagine, that changing the jquery code to dynamically add and remove tailwind classes will not improve the readability but rather make it worse.

HTML clutter might be the only downside of adding utility classes to web components. We're mainly manipulating the DOM by jQuery, and will mainly be reading the JS code, so I think adding these classes can make things easier, especially for different theme styling, which is at the moment really a pain in the back. In the future I am thinking about defining a set of custom pallettes for each of the themes, so that the work of adjusting themes can be reduced.

stickz commented 3 months ago

It seems a little late to pivot now but maybe this could be considered at another time.

Yes, we can address separation in v5.1. @jevenski Please feel to do whatever you need (within reason) to get v5.0 released with mobile functionality.

I'm reviewing for functionality and polish not code quality. We need this narrow scope for v5.0.

stickz commented 2 months ago

@jevenski Any ETA on addressing the white background of the responsive side panel for different themes? I have time to review again this weekend.

jevenski commented 2 months ago

Okay, I'll try to get it done before this weekend begins.

stickz commented 2 months ago

Merged, thanks!