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

Refactor embed.js.erb with layers.yml #5571

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hlfan
Copy link
Contributor

@hlfan hlfan commented Jan 29, 2025

Like #5550 but with reading the layers.yml file instead of duplicating information

new L.OSM.Mapnik(mapnikOptions).addTo(map);
}
const layers = <%=
(YAML.load_file(Rails.root.join("config/layers.yml"))
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe the outer parentheses are needed here? I tried it in the rails console and I got the same JSON without them...

Also can we indent the ruby code by four spaces so it's a bit more readable? or does that trigger some lint issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I tested it directly with puts, which didn't like the dangling to_json, but this isn't the case here.

Copy link

1 Warning
⚠️ Merge commits are found in PR. Please rebase to get rid of the merge commits in this PR, see CONTRIBUTING.md.

Generated by 🚫 Danger

@hlfan hlfan closed this Jan 30, 2025
@hlfan hlfan deleted the embed-unified-layers branch January 30, 2025 06:23
@hlfan hlfan restored the embed-unified-layers branch January 30, 2025 06:27
@hlfan hlfan reopened this Jan 30, 2025
@hlfan
Copy link
Contributor Author

hlfan commented Jan 30, 2025

Ok I don't think I could've failed harder in squashing the previous commits 🫠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants