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

added 2 columns: Size and Revisions and made all column headers sortable #20

Closed
wants to merge 33 commits into from
Closed

Conversation

HarryBleckert
Copy link

I added 2 columns: Size and Revisions to adminpads2 and made all column headers sortable.

For this pull request I created a new fork of adminpads2: https://github.com/HarryBleckert/ep_adminpads2
Changed files:
pads.js
pads.html
index.js
locales/en.json

Added files:
locales/de.json

Added folders
static/js/admin/js-cookie
static/js/admin/img

@JohnMcLear
Copy link
Member

Look at files changed to see what actually got changed. I think what you think you have done and what you actually have done are not the same.

@HarryBleckert
Copy link
Author

Look at files changed to see what actually got changed. I think what you think you have done and what you actually have done are not the same.

Good day John,
I agree, I am not experienced using Github and my pull requests were confusing and wrong.
I made a mistake with my first pull request merging all files from the fork I created.
Then before my 2nd pull request removed all unchanged files from that fork and only merged with the files I changed or added.

However, the new code is working very well and adds 2 more columns and sorting to all columns.

Please feel free to merge and see all changed and added files here:
ep_adminpads2.zip

Pasted from the updated README.md:
image

New features on Dec. 07, 2020

New columns: Pad size, No. of revisions
Pads are sorted: Default sorting: Last Edited, descending All columns can be sorted descending or ascending by clicking on column headers
Animated "Loading.." Icon
To be Done:

Currently up to 35,000 pads can be handled. On re-sorting padManager.listAllPads() ist called again. This should only happen a single time on startup.
Automatic Updates on pad changes don't work and the checkbox has been temporarily disabled.

@rhansen
Copy link
Member

rhansen commented Dec 10, 2020

Thank you for your work. Before this can be merged, a lot needs to change:

  • The commits are a mess. I rebased them onto latest main, but they need to be organized for readability. I highly recommend reading Pro Git to learn how to effectively use Git's interactive rebase feature.
  • There are many changes that must be reverted. In particular, undo the changes to translations and the deletion of important control files.
  • js-cookie should not be copied into the repo. Add it as a dependency in package.json instead.
  • Delete commented-out code.
  • TODO comments must be resolved or deleted.
  • The code must pass lint checks.

Before you spend any more time on this, please describe the approach you are taking (or would like to take) so that we can discuss. (Probably comment in #18, or open a new issue.) There are many existing JavaScript libraries that can be used here; if there is one that is a good fit for this I would like to take advantage of it.

I'm going to close this PR because I don't expect it to be ready for review any time soon. When you have addressed all of the above issues and believe it is ready, please open a new PR.

@rhansen rhansen closed this Dec 10, 2020
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.

3 participants