Skip to content
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

Tweak DHCP datatables #2042

Merged
merged 4 commits into from
Jan 5, 2022
Merged

Conversation

Moonlight63
Copy link
Contributor

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

Brings the DHCP page in settings lin line with the datatables everywhere else in (such as DNS records and Groups).

How does this PR accomplish the above?:

By using the same datatable configuration as is used everywhere else.

What documentation changes (if any) are needed to support this PR?:

None

@yubiuser
Copy link
Member

yubiuser commented Jan 1, 2022

Thanks for your submission. The tests fail because of the spaces after dom:

@DL6ER DL6ER changed the title Tweak/dhcp datatables Tweak DHCP datatables Jan 1, 2022
@DL6ER
Copy link
Member

DL6ER commented Jan 1, 2022

Before:
Screenshot from 2022-01-01 12-38-03

After:
Screenshot from 2022-01-01 12-38-20

@DL6ER
Copy link
Member

DL6ER commented Jan 1, 2022

You change makes the tables grow significantly in height. I'd suggest putting each table in their own box, like
pi hole_admin_settings php_tab=piholedhcp (1)

diff --git a/settings.php b/settings.php
index c9a15c96..e3e808de 100644
--- a/settings.php
+++ b/settings.php
@@ -614,12 +614,11 @@ if (isset($_GET['tab']) && in_array($_GET['tab'], array("sysadmin", "dns", "piho
                             <div class="col-md-12">
                                 <div class="box box-warning">
                                     <div class="box-header with-border">
-                                        <h3 class="box-title">DHCP leases</h3>
+                                        <h3 class="box-title">Currently active DHCP leases</h3>
                                     </div>
                                     <div class="box-body">
                                         <div class="row">
                                             <div class="col-md-12">
-                                                <label>Currently active DHCP leases</label>
                                                 <table id="DHCPLeasesTable" class="table table-striped table-bordered nowrap" width="100%">
                                                     <thead>
                                                         <tr>
@@ -648,10 +647,19 @@ if (isset($_GET['tab']) && in_array($_GET['tab'], array("sysadmin", "dns", "piho
                                                         <?php } ?>
                                                     </tbody>
                                                 </table>
-                                                <br>
                                             </div>
+                                        </div>
+                                    </div>
+                                </div>
+                            </div>
+                            <div class="col-md-12">
+                                <div class="box box-warning">
+                                    <div class="box-header with-border">
+                                        <h3 class="box-title">Static DHCP leases configuration</h3>
+                                    </div>
+                                    <div class="box-body">
+                                        <div class="row">
                                             <div class="col-md-12">
-                                                <label>Static DHCP leases configuration</label>
                                                 <table id="DHCPStaticLeasesTable" class="table table-striped table-bordered nowrap" width="100%">
                                                     <thead>
                                                     <tr>

@Moonlight63
Copy link
Contributor Author

Oh, OK. I didn't catch that. Yeah, I think you're right, it looks better with each table in its own box. I'll submit a new PR in the morning!

@DL6ER
Copy link
Member

DL6ER commented Jan 1, 2022

No need to submit a new PR. You can just add additional commits to your existing branch and once you push them they will automatically appear here.

Signed-off-by: Dalen <dalencattmlsp@gmail.com>
@Moonlight63
Copy link
Contributor Author

On another note. I have noticed that a lot of files have gotten really big and monolithic, well beyond the point where I would have been splitting them up. The settings page is a good example. I would be happy to undertake the challenge of refactoring some of these files breaking them up into smaller more maintainable chunks, but I also don't want to step on any toes. I don't know how refactoring large files would affect any open PRs or branches. I am coming mostly from a Vue background, so I am used to breaking everything up into the smallest possible components. Maybe this isn't as big of an issue with PHP. I just know looking at this huge file gives me anxiety 😅

@DL6ER
Copy link
Member

DL6ER commented Jan 2, 2022

Thanks for your suggestion. Behind the scenes*, we are developing v6.0 which is currently looking at dropping PHP altogether. This will make the current web interface even more unmaintainable as a lot of code is being duplicated to get rid of the include and require statements. However, that's the price we are going to pay for it.

The gain is a much more robust server that also has a much nicer REST API. In a follow-up step, after we have the much nicer API, we want to look into starting fresh with an altogether new web interface that finally solves all these limitations the current web interface opposes.

One of our main developers, @dschaper has already looked into Vue, if you'd be able to help us creating a new interface from scratch, that'd be totally awesome and immensely helpful as we have not done anything more than one or two tutorial (at best!) and don't have any real experience we could built on (like best practices, etc.).

It's not something that will happen just now, but I do think its something that may start sometime this year (no promises!). We first have to finish the unPHPization and finish the new API. I hope to be able to continue this soonish but we will have to see how the next months look like in terms of time for the hobby.

Having said that, I think it'd actually be disadvantageous if you'd start a massive rewrite of the code base right now. Both because I'm used to the monolithic files and because I'd have to undo most of your changes anyway.

Would you be ready to contribute ideas (and hopefully code) when we decide to explore Vue as the web interface for Pi-hole?

@Moonlight63
Copy link
Contributor Author

Moonlight63 commented Jan 2, 2022

Yes, absolutely. I'd be more than happy to help with that in between school and such. I may be biased, but I think moving to vue is much better for maintainability and flexibility. I'd certainly like to contribute where I can. Vue is really not that hard, I think you could pick it up rather quickly. I'd be willing to help to get the ball rolling. If done well, I think the overall performance may actually be better as well. It depends on the implementation, but you could either build the web server to let the rendering be done client side, or I think with vue 3 you can also do a sort of "pre-compile" step so that the web server just serves static files. I probably wouldn't do server side rendering because pi-hole is still a project built for running on a raspberry pi. Probably best to keep the server side workload as low as possible. Anyway, I digress. Yes, I'd love to help. Lol

If you want you can hmu on discord and we can talk more about it: Moonlight#8582

@PromoFaux PromoFaux requested a review from DL6ER January 4, 2022 18:57
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/adjustable-height-of-dhcp-and-static-leases-tables-in-settings/52220/2

@Moonlight63
Copy link
Contributor Author

Well... There I go thinking about a project and can't stop myself. I started playing around making some Vue components and got to here:
Screenshot 2022-01-05 at 04-32-57 Dashboard — Pi-Hole Admin Pro

Then I started toying with the idea of building a dynamic theme engine for Tailwind to maintain that functionality. It basically works!
Screenshot 2022-01-05 at 04-38-33 Dashboard — Pi-Hole Admin Pro

Anyway, it's not functional yet. The data is just pulling from a dummy API generator. I figure I could build in a data fetch system with vuex that could connect to a websocket running on the pihole webserver, and make it fall back to long polling worst case scenario. Point is once the components are made data can be piped in pretty simply.

This is obviously a completely disconnected topic to the PR, but since it was mentioned I wanted to give you an idea of what is possible.

@DL6ER
Copy link
Member

DL6ER commented Jan 5, 2022

Thanks, this is nice. I don't know how sensible it'd be to code around the current API. Better directly fit it to the new v6.0 API when it is feature complete (well, it can already do the entire dashboard main page)

@DL6ER DL6ER merged commit eadd6c5 into pi-hole:devel Jan 5, 2022
@PromoFaux PromoFaux mentioned this pull request Jan 5, 2022
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-13-web-v5-10-and-core-v5-8-released/52254/1

@DL6ER
Copy link
Member

DL6ER commented Jan 14, 2022

@Moonlight63 would you be willing to share your example code with us so we could use is as one option for the next-gen redesign of the Pi-hole dashboard? Works there be any licensing restrictions for us to consider?

@Moonlight63
Copy link
Contributor Author

Yes of course! That was always the plan. I am still working on it but I haven't had a chance to do as much as I would have liked this week. Right now I am working on making a better datatable with sorting and fuzzysearch. After that, I want to start working on getting some type of API integration through vuex. I've been mostly trying to clean up my workflow and refactoring some code to make things a bit more modular. It's based on AdminOne, but by the time I have finished refactoring it will be pretty different, the only thing that will really be left is the tailwind UI. I am hoping to finish with making the data table and having an actual working demo in a week or two. We'll have to see how school goes!

https://github.com/Moonlight63/Pihole-Admin-Pro

If you need a good starting place for Vue development, I set my VSCode up with 3 base plugins, Settings Sync, Settings Cycler, and Extension Profiles. With Extension Profiles I create a new profile for Vue development, then I create a matching "settings.cycle" block in my settings.json with the same ID that overrides the "sync.gist" option. I give it a gist ID that I have already set up previously. I made this one public for you so you can use it.
https://gist.github.com/Moonlight63/99a0860f6c3aafa679b4360ba32379dd
Just run Sync: Download or Sync: Advanced -> Download from public Gist inside your Vue-Dev Extension Profile and it will set you up with a dev environment for Vue 3 and tailwind.

In case you have never used devcontainers in vscode before, here's a quick rundown. Inside the admin repo, I have also added a devcontainer folder. You can use that with the Remote - Containers plugin and Docker to spin up a container that will automatically install all needed dependencies and vscode will open inside the container. VSCode will probably ask you the first time if you want to reopen the folder inside the container if you have the plugin installed, just hit yes and as long as you have Docker it will start building the container. It will take a few minutes to build on the first go and may seem like it's frozen, but it's not. This will keep development consistent between anyone working on the project and will keep your host machine clean. Once you are inside the container you can open the vscode terminal which will be the terminal inside the container and run 'npm run serve' to get the node dev server running.

Also, if you want to use Vue devtools in your browser, currently you need to install the v6 beta from github:
https://github.com/vuejs/devtools/releases
I am using firefox for development and debugging, so all you have to do is click on the .xpi file and it will install the beta. The version from the extensions store in both chrome and firefox is not working with Vue 3.

I kinda figured once I get it to a working 'alpha' state, I can transfer the repo over, or you could clone it. If you need help or want me to walk you through the code or what I'm doing feel free to add me on discord.

@dschaper
Copy link
Member

Can you shoot me an email at dan.schaper@pi-hole.net, I'd like to pass over an invite to our Mattermost setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants