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

Code cleanup #50

Closed
wants to merge 11 commits into from
Closed

Code cleanup #50

wants to merge 11 commits into from

Conversation

ZiClaud
Copy link
Contributor

@ZiClaud ZiClaud commented Sep 1, 2024

Issue #45

- changed some var to const to increase efficiency
- closed </div> and </button> in outline.ejs and stats.ejs
- put === instead of == inside of stats.js
- Removed main.css
- Code inside main_node.js was a duplicate of main.js, removed
- CSS files improved
- Switching from == to ===
- Minor code improvement
- Deleted main_node.js - it was a copy of main.js
@himanshunaidu
Copy link
Collaborator

himanshunaidu commented Sep 1, 2024

There are far too many different kinds of changes being put together in one PR here.
We are all already working on separate PRs for and this is going to conflict a lot with every one of those.
I get the idea behind the issue you raised, but we need to be very careful while trying to push these changes.
It'd have been better if we raised separate PRs for linting issues, splitting the css files, removing main_node and other files (which I am not sure are redundant btw, please check with @thuiop on this), etc.

@@ -5,7 +5,7 @@
"license": "MIT",
"main": "main.js",
"scripts": {
"start:server": "nodemon main_node.js",
"start:server": "nodemon main.js",
Copy link
Owner

Choose a reason for hiding this comment

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

No, this is not right. The "main.js" here is the Electron one, the "main_node.js" is the one for the server. Please also restore the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the two files have diverged quite a bit. And it may be confusing later on to keep the two files in sync.
For now, can we simply do something like: Have some comments on the main.js file, pointing to which part is related to electron and which is not? It will make it easier to copy paste the contents accordingly, until we try make them in sync automatically.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, there is some issue there. I will try to have a shared common root there so that there are no such issues.

</button>
</div>
<% if (locals.path === "/saves") { %>
<div id="content" class="content" hx-post hx-trigger="load" hx-ext="submitlocalstorage">
Copy link
Owner

Choose a reason for hiding this comment

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

The linting does not have the correct indentation here.

Hearing:
<span id="hearing_value">
<% if (locals.v_hearing) { %> <%= v_hearing %> <% } else {%> 0 <% } %>
<% if (locals.v_hearing) { %> <%= v_hearing %>
Copy link
Owner

Choose a reason for hiding this comment

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

This style is arguably less readable than the previous one. Also all of these could be collapsed to something like

Suggested change
<% if (locals.v_hearing) { %> <%= v_hearing %>
<%= locals.v_hearing || 0 %>

(I think)

@ZiClaud
Copy link
Contributor Author

ZiClaud commented Sep 1, 2024

Alright, will work on fixing it, also I'll add some documentation on main_node.js

@ZiClaud
Copy link
Contributor Author

ZiClaud commented Sep 1, 2024

Will create a new Pull Request

@ZiClaud ZiClaud closed this Sep 1, 2024
@ZiClaud ZiClaud deleted the current branch September 10, 2024 08:14
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