netbox
netbox copied to clipboard
Refactor row coloring logic and simplify mark planned/connected toggle implementation
Fixes: #13712 and #13806.
As I mentioned in the discussion of #13712, the root cause of the fact that this bug happens in the first place is that the current approach in Netbox to toggling cable state implements styling logic for interface rows in two distinct places, one which is server-side by applying CSS classes to table rows depending on different criteria, and again on the client-side by updating the DOM after the fact.
These two implementation had slipped out of sync at some point, which caused the bug to appear.
To fix the root problem, which is a duplication of logic, I have refactored the row coloring logic to fully happen in CSS. This makes sense, because setting colors of table rows is styling, so it makes sense to do that in CSS. To do this, the following enhancements needed to be made:
- Adding data attributes to table rows representing virtual interface type, mark connected, as well as cable state. This is so that we can define CSS attribute selectors for said interface connection states.
- Creating CSS variables for colors for choices. This is so that these colors can then be referenced by name in the runtime-generated CSS for styling rows.
- Dynamically generating stylesheets based on available choices. This has to happen at runtime because connection states are potentially user-customizable.
This allowed the code implementing the toggle button for cable state to be substantially simplified. Instead of generating a single button at template render time depending on the current cable state, both buttons are now just always generated, and hidden/shown depending on the current state of the cable (as per the attribute on the table row). Visually the effect is the same, and code-wise, it's a lot simpler. The only element the Typescript current twiddles in the DOM is now the data-cable-status attribute of the table row, the CSS does the rest, including showing the correct buttons and coloring the rows.
Now, this pull request is likely not acceptable in its current state. A few issues I've already identified:
- Cable connection states are not only togglable in Device Interface Views, but in several other places. For consistency, this should be updated across the board. In fact, I've probably broken these buttons in other views in the current state of the PR.
- I'm not sure if I've added the new CSS styles in the right place. The "choice-based" colour styles by neccessity need to be generated at runtime and putting them in the dcim/device/interfaces.html template's head block was the easiest solution. I'm open for any opinions on whether these CSS styles need to be moved. For now I just did the simplest thing that could possibly work as a proof-of-concept, but ultimately this is a design decision.
Anyway, before I put in any more work into this PR to finish it, I'd like some feedback at least on the above points, and whether you agree with the general direction I'm going with this, because it turns out this was much more than a simple bugfix. If you're looking for a simpler "bugfix" type change that doesn't address the underlying issue, there's also my first stab at it, at #13807, but that's just likely to break again because it doesn't address the repetition of logic.
This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.
This PR has been automatically closed due to lack of activity.
@jeremystretch Marked you as review requested as peter would like some feedback before he pushes forward.
peter would like some feedback before he pushes forward.
Thanks for raising this for me, but my name is Per, not Peter.
Any maintainer is capable of reviewing this; there is no need to ping me. However, I'll point out that this is still a draft PR. If it's ready for review, its conflicts with the base branch and all tests must pass, and it needs to be marked as such.
Looking back at this after a few months, another bug jumps out at me that I didn't notice, and it probably present both in this new implementation and the existing implementation - if you have a listing with both ends of the cable (e.g. both interfaces where both of them connect to each other) the background colour only gets updated on the interface you actually click, not the other one.
Just putting this here as a note, if it's an easy fix I might throw it in "for free".
Edit: Nope, it's not easy to fix "for free", I won't include a fix for this.
I've adjusted this PR to align with the guidance provided by @DanSheps and I think it'll be closer to acceptable now.
As for what I wrote before:
Cable connection states are not only togglable in Device Interface Views, but in several other places. For consistency, this should be updated across the board. In fact, I've probably broken these buttons in other views in the current state of the PR.
I'm not sure what I was thinking back when I wrote it, but I can't find any other places where interfaces are coloured this way so... I think I'm done? At least functionality-wise.
Took a quick look and seems to be in-order. I will pull it down when I can and do a more in-depth dive into it.
@DanSheps did you get a chance to look into / review this more?
No, but thanks for the reminder. Will take a look toninght/tomorrow morning hopefully
Tested, works very well.
Thanks for the work @pv2b