-
Notifications
You must be signed in to change notification settings - Fork 282
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
Clean up archive.html and archive.js #1290
Conversation
Great effort @7malikk. |
Thanks @segun-codes |
let imageRow = document.createElement('div'); | ||
let image = new Image(150, 150); | ||
let placeButton = document.createElement('a'); | ||
response.data.files.forEach((file) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. I'm going to merge it -- maybe a next step could be to break out lines 70-96 in a separate function called, what do you think... renderImages
? or would it be better to have separate ones like renderPng()
and renderJpeg()
? Yeah, maybe that sounds better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that sounds a lot better @jywarren
I'd add that next. Thanks for the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just going through this again, I think renderImages
would be better as there is no distinction when we render a .png or .jpeg file as noticed here:
Leaflet.DistortableImage/examples/js/archive.js
Lines 69 to 92 in 6f925da
response.data.files.forEach((file) => { | |
if (file.format === 'PNG' || file.format === 'JPEG') { | |
const imageRow = document.createElement('div'); | |
const image = new Image(150, 150); | |
const placeButton = document.createElement('a'); | |
fetchedFrom = document.createElement('p'); | |
fetchedFromUrl = document.createElement('a'); | |
fetchedFromUrl.setAttribute('href', input.value); | |
fetchedFromUrl.setAttribute('target', '_blank'); | |
fetchedFromUrl.innerHTML = 'this Internet Archive Collection'; | |
fetchedFrom.appendChild(fetchedFromUrl); | |
placeButton.classList.add('btn', 'btn-sm', 'btn-outline-secondary', 'place-button'); | |
placeButton.innerHTML = 'Place on map'; | |
image.src = `${url.replace('metadata', 'download')}/${file.name}`; | |
imageRow.classList.add('d-flex', 'justify-content-between', 'align-items-center', 'mb-4', 'pe-5'); | |
imageRow.append(image, placeButton); | |
imageContainer.appendChild(imageRow); | |
imageCount++; | |
} | |
}); |
There is no different render method depending on file format.
What do you think @jywarren?
that sounds good! Thanks!
…On Sat, Dec 10, 2022 at 4:23 PM Malik ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/js/archive.js
<#1290 (comment)>
:
> .then((response) => {
if (response.data.files && response.data.files.length != 0) {
let imageCount = 0;
- response.data.files.forEach(file => {
- if (file.format === 'PNG' || file.format === 'JPEG'){
- let imageRow = document.createElement('div');
- let image = new Image(150, 150);
- let placeButton = document.createElement('a');
+ response.data.files.forEach((file) => {
I was just going through this again, i think renderImages would be better
as there is no distinction when we render a .png or .jpeg file as noticed
here:
https://github.com/7malikk/Leaflet.DistortableImage/blob/b396b5a0023183605fa4354626c886638659bf34/examples/js/archive.js#L69-L92
There is no different render method depending on file type.
What do you think @jywarren <https://github.com/jywarren>?
—
Reply to this email directly, view it on GitHub
<#1290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J4YXFLXZ3U7SRAJBZ3WMTYEFANCNFSM6AAAAAAS2EYYII>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
Code cleanup in terms of indentation, link, script, and meta spacing and ensuring no code break and maintained functionality.
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
grunt test
If tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!