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

add basic web UI #1395

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

paigewilliams
Copy link

@paigewilliams paigewilliams commented Jul 1, 2024

I branched off of #1142 to address the PR feedback from @nyurik .

Let me know if there is a better way to submit these changes for review.

@nyurik
Copy link
Member

nyurik commented Jul 2, 2024

thx! I will be able to review it fully next week (I'm at FOSS4G conference). One thing -- is it possible to make martin-ui/src/_assets/logo.png into a symlink? I don't think we should duplicate files in the repo unless we have to.

@nyurik
Copy link
Member

nyurik commented Jul 2, 2024

P.S. please merge in the latest main branch

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

a few minor changes, plus I merged and pushed the latest main branch. One thing -- for some reason, CI fails for this PR - need to fix that

README.md Outdated Show resolved Hide resolved
martin/build.rs Outdated Show resolved Hide resolved
Comment on lines 8 to 9
// assets can also be the name of a tile source
// so we use _assets to avoid conflicts
Copy link
Member

Choose a reason for hiding this comment

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

Looking at all the requests when viewing localhost:3000/ while setting RUST_LOG=debug, I see these requests:

[2024-07-09T14:32:23Z INFO  actix_web::middleware::logger] 127.0.0.1 "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" 0.000332
[2024-07-09T14:32:23Z INFO  actix_web::middleware::logger] 127.0.0.1 "GET /assets/index-0vhmA-Yl.js HTTP/1.1" 304 0 "http://0.0.0.0:3000/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" 0.000802
[2024-07-09T14:32:23Z INFO  actix_web::middleware::logger] 127.0.0.1 "GET /_assets/index.RjhygAlW.css HTTP/1.1" 304 0 "http://0.0.0.0:3000/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" 0.001011
[2024-07-09T14:32:23Z INFO  actix_web::middleware::logger] 127.0.0.1 "GET /_assets/logo.ZTiqb92t.png HTTP/1.1" 304 0 "http://0.0.0.0:3000/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/126.0.0.0 Safari/537.36" 0.000364

So it clearly gets BOTH /assets and /_assets -- not good. Moreover, it IS possible to create an _assets source, so it could conflict. The only way to NOT have any conflicts is to add all assets to /_/assets/* path -- this way it will never conflict (the _ is a reserved keyword)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure that this PR is complete. When I run npm run build && npm run dev in the martin-ui sub-directory, the UI runs fine. When I run cargo run -- ./world_cities.mbtiles, local is unable to find the js file at the root, but is able to find a css file at /assets. I'm not sure what is happening at cargo run ... that would prevent it from finding the js file.

Copy link
Member

Choose a reason for hiding this comment

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

why would anything access things at /assets/...?

Copy link
Author

Choose a reason for hiding this comment

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

I think that my latest change meets what you are looking for with js, css and image files all served from _/assets. Running cargo run -- ./world_cities.mbtiles now runs the web UI at /.

If it doesn't, can you outline the directory or output build structure you have in mind?

@nyurik nyurik changed the title web UI with some feedback addressed add basic web UI Jul 12, 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

Successfully merging this pull request may close these issues.

None yet

3 participants