Skip to content

introduce typescript - #447

Open
jgdhs27 wants to merge 1 commit into
n-rook:mainfrom
jgdhs27:ts
Open

introduce typescript#447
jgdhs27 wants to merge 1 commit into
n-rook:mainfrom
jgdhs27:ts

Conversation

@jgdhs27

@jgdhs27 jgdhs27 commented Aug 13, 2023

Copy link
Copy Markdown
Collaborator

Adding types here to make the logic easier to follow

@n-rook n-rook left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Nice; using ts will be really convenient and make it easier to maintain the frontend without getting lost in the complexity of it.

<script>
const showGameColumn = "{{show_game}}";
const showRouteColumn = "{{show_route}}";
const aaaa = "aaaaa";

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

?

@@ -1,205 +1,200 @@
"use strict";

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sorry to be annoying by saying this, but: We really shouldn't check in generated code. Rather, we should recreate it whenever we deploy.

If you really need to, it might be okay to check it in as an intermediate step while you're trying to get the ts deploy working, but if so, please put a giant comment at the top warning that this is generated code. But it'd be way better to do it in one go.

}

function ensureTickboxSync(): void {
activeFilters = initializeFilters();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Er--- why are we calling initializeFilters() three times in this module?


json_dict = {}
json_dict["Id"] = replay.id
json_dict["id"] = replay.id

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

IMO you should really make these changes in a separate PR; they make this one look much bigger than it needs to be

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