-
-
Notifications
You must be signed in to change notification settings - Fork 610
Improve DHCP static leases management GUI #3565
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
base: development
Are you sure you want to change the base?
Conversation
…xtarea below Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
…t any possibility for code injection) Signed-off-by: DL6ER <[email protected]>
The video looks good to me, but unfortunately I'm not able to test this as I have now stopped using PiHole (partly due to this very issue). However, two comments based on the video: |
Signed-off-by: DL6ER <[email protected]>
Yes, this is expected and per design. Advanced config lines may include
I did add |
Signed-off-by: DL6ER <[email protected]>
If something invalid is entered in the advanced box (e.g. The live-editing is cool but only shown advanced > table, not the other way round. Is this by design?
This could be hard to find if n is big. Could we show line numbers in the advanced text box? |
Signed-off-by: DL6ER <[email protected]>
I tested using an invalid IP (
When saved, the invalid IP is ignored, resulting in I think this is the correct behavior, but no warning was returned to the user, to inform something was wrong and the entry was "fixed" to fit the valid input fields. Maybe we need a warning/alert for cases like this. |
Signed-off-by: DL6ER <[email protected]>
Like so? Screencast.From.2025-07-20.1.webm |
Signed-off-by: DL6ER <[email protected]>
Screencast.From.2025-07-20.20-39-42.webm |
#StaticDHCPTable td.table-danger { | ||
background-color: #ff000022 !important; | ||
color: #fff !important; | ||
} | ||
|
||
#StaticDHCPTable td.table-success { | ||
background-color: #74c70022 !important; | ||
color: #fff !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick suggestion just to improve the colors:
#StaticDHCPTable td.table-danger { | |
background-color: #ff000022 !important; | |
color: #fff !important; | |
} | |
#StaticDHCPTable td.table-success { | |
background-color: #74c70022 !important; | |
color: #fff !important; | |
} | |
#StaticDHCPTable .table-danger { | |
background-color: #d337; | |
} | |
#StaticDHCPTable .table-success { | |
background-color: #2c24; | |
} |
Still using semi-transparent colors, but now the contrast is good enough when using the original text color, so I think we don't need to change color:
.
Also, we should use !important
only in cases where we really need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…hemes Signed-off-by: RD WebDesign <[email protected]>
I added one commit to change the high contrast themes. On these themes the colors are not as important as the contrast/readability. |
Note: I was testing with DHCP server disabled, so I'm not sure if we need to fix this, but when you use the textearea to enter a line, if there are invalid values on this line, the values are not checked and the API accepts everything, including the invalid values. The first line contains an invalid IP:
|
Signed-off-by: DL6ER <[email protected]>
Signed-off-by: DL6ER <[email protected]>
Yes, invalid |
Signed-off-by: DL6ER <[email protected]>
The line numbers in the |
The line numbers issue is fixed. Note: I'm not sure if I like the idea of removing the textarea resizing, but this is already doing what most users requested. We can think later about a solution for the resize. I found another small issue: There is no hostname validation. If you enter a hostname containing spaces (or not allowed characters) the hostname will be saved. I tested using |
Personally I think the text "Please save this line before editing another or leaving the page, otherwise your changes will be lost." is a little misleading because even if the user saves the line (using the green button), the changes will still be lost if the user leaves the page without clicking on "Save & Apply". I'm still unsure how to change this. I had 2 different ideas:
|
I agree with rd, it's not clear that the user needs to click "Save & Apply" after they already pressed the save icon |
This comment was marked as off-topic.
This comment was marked as off-topic.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
What does this implement/fix?
See title. This adds a easy accessible table that is kept in sync with the
<textarea>
which still allows for any advanced configuration.Please see this movie for a demonstration:
Screencast.From.2025-07-15.12-41-22.webm
You can test all of that even without having the DHCP server enabled. What will be saved in the end is always the content of said
<textarea>
which is getting stores asdhcp.hosts
inpihole.toml
.Related issue or feature (if applicable): N/A
Pull request in docs with documentation (if applicable): N/A
By submitting this pull request, I confirm the following:
git rebase
)Checklist:
development
branch.