-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Refactor row coloring logic and simplify mark planned/connected toggle implementation #13874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Preparatory work for factoring row styling decisions out of Python code.
Preparatory work for moving row styling to CSS
Preparatory work for factoring row styling out of Python
Preparatory work for simplifying toggle button code for cable status.
|
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. |
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:
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 |
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:
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:
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.