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

[Discussion] Cleaner code #45

Open
ZiClaud opened this issue Aug 31, 2024 · 4 comments
Open

[Discussion] Cleaner code #45

ZiClaud opened this issue Aug 31, 2024 · 4 comments

Comments

@ZiClaud
Copy link
Contributor

ZiClaud commented Aug 31, 2024

Most logic needs to change, I'm changing some stuff with the .css but I can't understand how stuff like the header works.

  • Header appears where it shouldn't (on the Menu and Stats page).
  • Same thing with footer, I'm currently removing it from everywhere except the menu, and I needed to change way more than I wanted to.
  • Many functions use "==" instead of "===" which may cause bugs in the future.
  • main.css is not used
  • In utils.js we have navigateTo(url) not used

The worst thing is that there's no clear distinction from back-end and front-end, I kinda feel like we should clean the code before continuing, maybe put some rules.

For example, at the end of most of the .ejs there is this:

<div style="display:none">
    <h2 id="header" hx-swap-oob="true">
        <%= id %>
    </h2>
</div>

Why is it at the end if it's a header? And what does it do, exactly? Why is there "style" on the .ejs?

I'd put many .css, stuff like header.css, footer.css, text.css, etc. Instead of having them all inside main.css


Something cleaner would be:

<div class="header">
    <h2>
        Book <%= book_id %> - Chapter <%= chapter_id %>
    </h2>
</div>

So that if we want to change stuff it's easier, like switch from that to:

<div class="header">
    <h2>
        Book <%= book_id %>
    </h2>
	<h3>
		Chapter <%= chapter_id %>
	</h3>
</div>

(Honestly idk if we the stuff inside <%= %> is possible or completely wrong, let me know)

@thuiop
Copy link
Owner

thuiop commented Aug 31, 2024

Yes, there is a lot of stuff to clean indeed.

Why is it at the end if it's a header? And what does it do, exactly? Why is there "style" on the .ejs?

This is an out-of-band swap. When pages are loaded, everything within the div #content is swapped by the response of the server. However, we also want to change the header, and the header is not inside #content. Therefore, we use the hx-swap-oob tag, which will swap #header with the content of the div. The style is there because this is not an element meant to be rendered in any case; in fact it is typically deleted by HTMX. It used to be visible when loading the page for the first time, hence the display:none, but I think it is not anymore (does not hurt to keep it anyway).

Header appears where it shouldn't (on the Menu and Stats page).

What do you mean? I cannot see why the header should not appear on those pages?

Same thing with footer, I'm currently removing it from everywhere except the menu, and I needed to change way more than I wanted to.

Not sure why you want to avoid the footer either, but it is really has simple as moving the div within the one for the menu (and probably changing the styling a bit)

The worst thing is that there's no clear distinction from back-end and front-end

I am not sure what you mean there. When moving to a new page or fetching new content, the front-end makes a query; that is all there is to it.

@ZiClaud
Copy link
Contributor Author

ZiClaud commented Aug 31, 2024

What do you mean? I cannot see why the header should not appear on those pages?

I mean, it doesn't appear on the app, also I always try to click "Main menu" again to leave the menu lol

Not sure why you want to avoid the footer either

I want to remove it since it appears only on the menu on the main app, also to give it a bit of a cleaner look

but it is really has simple as moving the div within the one for the menu (and probably changing the styling a bit)

It's not, since moving it will give it padding, since the padding was added to all the content, not to the single elements, working on fixing it

I am not sure what you mean there. When moving to a new page or fetching new content, the front-end makes a query; that is all there is to it.

Sorry if I didn't explain myself well, but stuff that's in files like stats.js:

<% if (locals.v_magical_power) { %> <%= v_magical_power %> <% } else {%> 0 <% } %>

and

<% function sceneAfter(id) {
    const regex = /(B(?<book>[0-9]*)-)?Ch(?<chapter>[0-9]*)[a-c]-.*$/
    if (result=regex.exec(id)) {
        return (result.groups["book"] == "3") && (parseInt(result.groups["chapter"]) >= 4)
    }
} %>
<% if (sceneAfter(locals.v_current_scene)) { %>

just feel wrong to see in the middle of html, I feel like there should be a cleaner way to do it.

@thuiop
Copy link
Owner

thuiop commented Aug 31, 2024

Sure, but this is a web app, we do not have to copy the original 1 to 1. Having the header always visible allows you to switch directly from the menu to the stats for instance, which I think is better for navigation. The footer does not do much though.

just feel wrong to see in the middle of html, I feel like there should be a cleaner way to do it

This is how templating works though? It can definitely be made cleaner but this is regular stuff.

@ZiClaud
Copy link
Contributor Author

ZiClaud commented Aug 31, 2024

If that can be made cleaner that'd be perfect

@ZiClaud ZiClaud mentioned this issue Sep 1, 2024
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

No branches or pull requests

2 participants