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

Remember last used sort column/direction on DHCP lease tables #1332

Merged
merged 3 commits into from May 25, 2020

Conversation

PromoFaux
Copy link
Member

By submitting this pull request, I confirm the following:

  • 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, and have included unit tests where possible.
  • 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)

What does this PR aim to accomplish?:

Reference here.

Remember the last used sort column/direction for the DHCP lease tables. Does what it says on the tin.

Is there any value in adding this flag to other datatables, do you think?

@PromoFaux PromoFaux requested review from XhmikosR and DL6ER May 19, 2020 06:57
@DL6ER
Copy link
Member

DL6ER commented May 22, 2020

Is there any value in adding this flag to other datatables, do you think?

All other tables should already do it.

@PromoFaux
Copy link
Member Author

Shows what I know!

image

Question though, I notice you are doing this with the groups tables:

 stateSaveCallback: function (settings, data) {
      // Store current state in client's local storage area
      localStorage.setItem("groups-clients-table", JSON.stringify(data));
    },

Is that actually needed, and should I add it to the DHCP table in this PR?

@DL6ER
Copy link
Member

DL6ER commented May 22, 2020

@DL6ER
Copy link
Member

DL6ER commented May 22, 2020

It resets the page and search fields

@PromoFaux
Copy link
Member Author

OK, done that, also whilst I was in there, I did some tidying up.

@@ -73,7 +73,7 @@
</div>
</div>

<script src="scripts/pi-hole/js/groups-common.js"></script>
<script src="scripts/pi-hole/js/common-utilities.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, I have this in #1346.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha. utils is a better name.

@XhmikosR
Copy link
Contributor

I'd rather we merge first #1346 and then rebase this.

@PromoFaux
Copy link
Member Author

Fine by me

… direction/column

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@XhmikosR XhmikosR force-pushed the tweak/dhcp_save_state branch 3 times, most recently from 86b846f to 0b2483b Compare May 23, 2020 08:03
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member Author

OK, this appears to be working the same as it was before rebase. Just needs someone else to review it now

@XhmikosR
Copy link
Contributor

@PromoFaux how about moving the duplicate code patch to a separate PR? There are more duplicated stuff I want to move to utils but I don't want to overlap with this PR 🙂

BTW I believe we could squash the first 2 patches into one?

@XhmikosR XhmikosR mentioned this pull request May 25, 2020
9 tasks
@PromoFaux
Copy link
Member Author

To be honest that seems like doing more work for the sake of keeping things "just so"

Let's keep it in this PR. Also I'm not the biggest fan of rewriting history in git for the sake of minimising the number of commits. It's the destination that is important, not the journey.

Regards your PR with the other deduplication that is fine, just rebase on this branch (or devel once this is merged) - as it will land, it just needs for someone else to approve and merge it.

@XhmikosR
Copy link
Contributor

Well, to be fair they shouldn't be 2 patches in the first place 😛

Anyway, as you wish, I can rebase my PRs when needed.

@DL6ER DL6ER merged commit 184c8e4 into devel May 25, 2020
@DL6ER DL6ER deleted the tweak/dhcp_save_state branch May 25, 2020 18:05
@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/dhcp-free-scrolling-between-lines/33747/5

@yubiuser
Copy link
Member

yubiuser commented Jun 4, 2020

Is there any value in adding this flag to other datatables, do you think?

I found one: Local DNS Records lacks it.

@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-5-1-released/35577/1

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.

None yet

5 participants