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

Grid history, Server stats, global code update #1

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Elliot67
Copy link

@Elliot67 Elliot67 commented Oct 8, 2020

Overview

Here is a list of the things I did on the project :

  • updated dependencies & bundling the front javascript with parceljs
  • updated global code base (mostly front)
  • added a tab system with 3 different tabs :
    • grid history
    • pixel grid (the actual board)
    • server stats

Grid History

I wanted a way to check the evolution of the board. I added a tab where you can see the history of the board.
Every day the back saves the current board if when it receive a board request from one user. If it doesn't the board isn't saved.
The backend stores the data in a file inside server/history/year/month.json. And the file is a simple JSON file listing all the days with the board and a timestamp.

Pixel Grid

Didn't change a lot about that. Just rewrote some functions with fresher javascript and changed the CSS a bit.

Server stats

I saw a TODO about showing the stats to the user, so I did it.

How to setup the project

Install the dependencies with npm install and then npm run start to start the back with npm run watch to bundle the js and use the hot reload.
If you want to build the js for production, you can with npm run build.

Even if the project isn't active, I hope you will be interested in my PR. I still need to make a lint check and I'm of course ready to answer to any of your questions, suggestions. 😄

@danyshaanan
Copy link
Owner

Hi @Elliot67!
This is pretty amazing! I've been wanting to have some recorded history for a while now (once even implemented something but threw it away). The project hasn't been moving lately, but I'm still here and still dedicated to it, and your efforts are appreciated!
I'm currently pretty busy, but I'll make it a priority to review everything you did so that we could iron out any details and merge whatever possible. I hope to get into it within the coming days 👍

@Elliot67
Copy link
Author

Hey @danyshaanan ,
Did you manage to get some free time to start reviewing the PR ?
Hit me up if you have any questions, I would be happy help 😉

@danyshaanan
Copy link
Owner

Haven't managed to get enough free time. Thanks for the reminder! I've made a note to get to it this weekend. Will surely have comments/questions :]

@danyshaanan
Copy link
Owner

Haven't forgotten, just been super busy. Will get to it soon hopefully!

@danyshaanan danyshaanan self-assigned this Nov 17, 2020
@danyshaanan
Copy link
Owner

Hi @Elliot67 .
I've done a high-level review the changes, and I want to conceptually separate them into a few different sets of changes:

  • Refactoring
  • JavaScript version upgrade
  • Dependencies upgrades
  • Build process
  • Grid History
  • Server stats

The Grid History part is, of course, the most major and important one, however, the way it is all done, it can not be separated from the other parts. A build process (like some parts of the javascript upgrades) is something I intentionally avoided here, and the rest of the refactoring (like some other parts of the javascript upgrades) is important and good, but something I'd usually rather do myself on a codebase I'm planning to mainly keep on maintaining myself. I find it important to decouple (as possible) dependencies upgrades from any other changes. The server stats is a nice to have feature, but not too important.

Over all, accepting these changes will require me to do some careful reverting and restructuring, and after starting and realizing the efforts needed, I can suggest a couple of other ways to move forward:

If you've exhausted your efforts here, I'm gonna just rewrite the Grid history feature with some help from your code. Then I'll probably do the much needed refactoring to (some) ES6. Alternatively, if you'd wish and if it's not too much effort, you could reform the PR into the minimal set of changes required for the Grid History feature only, and we could pick it all up from there. (ideally, recording the data and displaying it are two different and separate commits/PRs).

If you have any other ideas, I'd be happy to hear. Thanks!

@Elliot67
Copy link
Author

Hi @danyshaanan ,
First of all thanks for the review and the time spent on it.

When I started working on the project, it was mainly to see how it was running and I wanted to try adding the history feature.
After some commits I realized the codebase was not so fresh and needed a little clean up, that's why my commits are a little bit messy and affect multiple parts at the same times.
What's more, my new implementations are not super clean either, the PR is more an MVP than a finished project so I totally agree with your feedback, it need to be changed in a more organized way.

I'm quite busy right now and I'm afraid it's gonna be for a while, but I'm still ready to help and would love to reform the PR.
The dependencies upgrade and build process are certainly the first step, using a bundler like webpack or parcel is certainly the best option. I choose parcel for the PR because it was quick to implement, but it's maybe not the best choice.
Tell me what you would like to go with, and if you still wanna use jQuery and lodash. (from what I remember, lodash was not used a lot).

On the server side, I decided to save the grid once a day and check if it was saved every time the server receive a request. It's certainly not the best choice too. If no one trigger the server for the whole day, nothing will be saved. If you are able to call a js file once a day with a simple cron task, the history should be easier to implement and way more reliable.

Thanks for the reading and have a great day !

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.

2 participants