Skip to content

Move html from javascript to views #5616

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.

Sign up for GitHub

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

Jump to bottom
Open
gravitystorm opened this issue Feb 5, 2025 · 6 comments · May be fixed by #6063
Open

Move html from javascript to views #5616

gravitystorm opened this issue Feb 5, 2025 · 6 comments · May be fixed by #6063
Labels
refactor Refactoring, or things that work but could be done better

Comments

@gravitystorm
Copy link
Collaborator

We have an amount of html building being created using JavaScript code. For example, in leaflet.share.js there is a substantial amount of html construction:

openstreetmap-website/app/assets/javascripts/leaflet.share.js

Lines 37 to 53 in 7cf9bf0

$("<div class='btn-group btn-group-sm mb-2'>")
.appendTo($form)
.append($("<a class='btn btn-primary'>")
.addClass("active")
.attr("for", "long_input")
.attr("id", "long_link")
.text(I18n.t("javascripts.share.long_link")))
.append($("<a class='btn btn-primary'>")
.attr("for", "short_input")
.attr("id", "short_link")
.text(I18n.t("javascripts.share.short_link")))
.append($("<a class='btn btn-primary'>")
.attr("for", "embed_html")
.attr("id", "embed_link")
.attr("data-bs-title", I18n.t("javascripts.site.embed_html_disabled"))
.attr("href", "#")
.text(I18n.t("javascripts.share.embed")))

This makes it harder to find, and much harder to maintain, than having the html in a view like the rest of the site. It's also mostly unnecessary, since html like this isn't dynamic, it's the same for every user.

We should move all of this html into views, and use the javascript only for interactivity like changing content (e.g. taking results of an ajax query and putting them into the html) or to show/hide certain elements after UI interactions.
@gravitystorm gravitystorm added the refactor Refactoring, or things that work but could be done better label Feb 5, 2025
@Satya900
Copy link

Satya900 commented Feb 6, 2025

Please assign me this issue i would like to work on it

@HolgerJeromin
Copy link
Contributor

@Satya900 We do not work with assigns. Just go ahead! :)

@AntonKhorev
Copy link
Collaborator

For example, in leaflet.share.js there is a substantial amount of html construction: ...

Was done in https://github.com/openstreetmap/openstreetmap-website/pull/4335/files#diff-2b34a8a6e260ed3c05d38b8250e07e05617d32985d2250826373e7c2bce40d8f.

Was ignored by everyone.

@gravitystorm
Copy link
Collaborator Author

Was ignored by everyone.

That PR is marked as a draft, so I haven't reviewed it.

@AntonKhorev
Copy link
Collaborator

That PR was marked as draft after it was ignored for months and I gave up on maintaining it.

@AntonKhorev AntonKhorev mentioned this issue Mar 6, 2025
Merged
@matkoniecz
Copy link
Contributor

Would #4335 be reviewed if it would be updated so it works again?

@hlfan hlfan linked a pull request May 29, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
refactor Refactoring, or things that work but could be done better
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move html from javascript to views hlfan/openstreetmap-website
5 participants
@gravitystorm @matkoniecz @HolgerJeromin @AntonKhorev @Satya900