Skip to content

Enable multiple map colourings #8159

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

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

corradio
Copy link
Member

Issue

Description

Preview

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@github-actions github-actions bot added frontend 🎨 dependencies Pull requests that update a dependency file labels May 22, 2025
@corradio corradio changed the title Corradio/changeable map colors Enable multiple map colourings May 22, 2025
@corradio
Copy link
Member Author

corradio commented May 26, 2025

@tonypls it will probably make more sense for us to sit down for this in order to review.
Nonetheless, I'm running into some testing issues, because it seems like there is a specific typecheck action for non-frontend code, which includes e.g. the arrow generation code.
However, because the arrow generation code now depends on the frontend codebase to read colors, this typecheck action fails, because it doesn't include all code, and thus the imports fail.

Do you know why two different typechecking actions are performed? Why not typecheck everything (frontend + scripts) in one go?

cc @VIKTORVAV99 in case you have additional insights or ideas to bring to the table.

@VIKTORVAV99
Copy link
Member

@tonypls it will probably make more sense for us to sit down for this in order to review. Nonetheless, I'm running into some testing issues, because it seems like there is a specific typecheck action for non-frontend code, which includes e.g. the arrow generation code. However, because the arrow generation code now depends on the frontend codebase to read colors, this typecheck action fails, because it doesn't include all code, and thus the imports fail.

Do you know why two different typechecking actions are performed? Why not typecheck everything (frontend + scripts) in one go?

cc @VIKTORVAV99 in case you have additional insights or ideas to bring to the table.

I think for the typechecking it's different because the scripts include node APIs in the default types (the ones you don't need to import) and the frontend includes the Vite types neither are available in the others environment.

Not sure if there is a easy fix but you can always mock the color object that is used if needed.

@VIKTORVAV99
Copy link
Member

I can take a look at reviewing this later today/tomorrow but I have a massive PR from the hack to review + update the API so not sure if I'll actually have time.

@corradio
Copy link
Member Author

@tonypls it will probably make more sense for us to sit down for this in order to review. Nonetheless, I'm running into some testing issues, because it seems like there is a specific typecheck action for non-frontend code, which includes e.g. the arrow generation code. However, because the arrow generation code now depends on the frontend codebase to read colors, this typecheck action fails, because it doesn't include all code, and thus the imports fail.
Do you know why two different typechecking actions are performed? Why not typecheck everything (frontend + scripts) in one go?
cc @VIKTORVAV99 in case you have additional insights or ideas to bring to the table.

I think for the typechecking it's different because the scripts include node APIs in the default types (the ones you don't need to import) and the frontend includes the Vite types neither are available in the others environment.

Not sure if there is a easy fix but you can always mock the color object that is used if needed.

Would love some guidance / ideas here when you have time!

@corradio
Copy link
Member Author

corradio commented Jun 12, 2025

cc @madsnedergaard given that this PR also contains code that updates the arrow generation scripts (images were outdated and the script didn't output the optimised webp files).
I don't think it should merged, but maybe worth extracting the arrow gen down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants