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

Replace var with let/const #1160

Closed
5 tasks
Tammy-Ajoko opened this issue Oct 14, 2022 · 25 comments · Fixed by #1305
Closed
5 tasks

Replace var with let/const #1160

Tammy-Ajoko opened this issue Oct 14, 2022 · 25 comments · Fixed by #1305

Comments

@Tammy-Ajoko
Copy link
Contributor

Tammy-Ajoko commented Oct 14, 2022

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from underrepresented groups in free and open-source software!

We know that the process of creating a pull request is one of the biggest barriers for new contributors. This issue is for you 💝

If you have contributed before, consider leaving this one for someone new, and looking through our general help wanted issues. Thanks!

🤔 What you will need to know.

Nothing. This issue is meant to welcome you to Open Source :) We are happy to walk you through the process.

📋 Step by Step

  • 🙋 Claim this issue: Claim the issue by commenting. If someone else has claimed it, ask if they've opened a pull request already and if they're stuck -- maybe you can help them solve a problem or move it along!
💡 Learn how to claim 🙋

Claiming an issue

Unless the issue is marked as reserved for someone, you can just say "I'd like to try this!" and then you've claimed it - no need to wait for someone to assign it to you. Just be sure you link your pull request (PR) to this issue so we can see where your solution is.

And open one early if possible - even before you've completed it with additional commits - and others can help you figure out any issues you may face.

  • 📝 Update the file pixelorigin.html in the Leaflet.DistortableImage repository (press the little pen Icon) and edit the line as shown below.

See this page for some help in taking your first steps!

Below is a "diff" showing in red (and a -) which lines to remove, and in green (and a +) which lines to add:

-38 var map;
+38 let map;
-42 var trd = [33, 0];
+42 const trd = [33, 0];
-49  var positron = L.tileLayer('https://{s}.basemaps.cartocdn.com/light_all/{z}/{x}/{y}.png', {
+49 const positron = L.tileLayer('https://{s}.basemaps.cartocdn.com/light_all/{z}/{x}/{y}.png', {
-66 var pane = map.getPane('overlayPane');
+66 const pane = map.getPane('overlayPane');
-68 var paneCorner = document.createElement('div');
+68 const paneCorner = document.createElement('div');
-76 var crsMarker = L.marker(map.unproject([0, 0]), {
+76 const crsMarker = L.marker(map.unproject([0, 0]), {
-83 var imageOffsetLine = L.polyline([[0, 0], [0, 0]], { color: 'skyblue' }).addTo(map);
+83 const imageOffsetLine = L.polyline([[0, 0], [0, 0]], { color: 'skyblue' }).addTo(map);
-86 var pixelOrigin = map.getPixelOrigin();
+86 const pixelOrigin = map.getPixelOrigin();
-87 var imagePixelCoords = map.project(trd, map.getZoom())
+87 const imagePixelCoords = map.project(trd, map.getZoom())
-88  var imageOffset = img._image._leaflet_pos;
+88  const imageOffset = img._image._leaflet_pos;
  • 💾 Commit your changes

  • 🔀 Start a Pull Request. There are two ways how you can start a pull request:

  1. If you are familiar with the terminal or would like to learn it, here is a great tutorial on how to send a pull request using the terminal.

  2. You can also edit files directly in your browser and open a pull request from there.

  • 🏁 Done Ask in comments for a review :)

🤔❓ Questions?

Leave a comment below!

Is someone else already working on this?

We encourage you to link to this issue by mentioning the issue # in your pull request, so we can see if someone's already started on it. If someone seem stuck, offer them some help! Otherwise, take a look at some other issues you can help with. Thanks!

(This issue was created by First-Timers-Bot.)

@7malikk
Copy link
Collaborator

7malikk commented Oct 14, 2022

@TildaDares @cesswairimu @jywarren I'd like to convert this into an FTO, so the first-timers can have an issue to resolve.

@TildaDares
Copy link
Member

Thank you for opening this issue @Tammy-Ajoko. Maybe @7malikk and @Tammy-Ajoko can work together to properly format this into an FTO. Thank you!

@7malikk
Copy link
Collaborator

7malikk commented Oct 14, 2022

@TildaDares alright, thank you.
@Tammy-Ajoko let's proceed

@Tammy-Ajoko
Copy link
Contributor Author

@7malikk we could proceed with the right format for an FTO. If you know it you could pls comment it here so I can create a new issue

@7malikk
Copy link
Collaborator

7malikk commented Oct 15, 2022

@Tammy-Ajoko here is the template for the FTO. Copy and paste it in yours and ping me if you encounter any challenges. Cheers🎉

@Tammy-Ajoko
Copy link
Contributor Author

Thank you for opening this issue @Tammy-Ajoko. Maybe @7malikk and @Tammy-Ajoko can work together to properly format this into an FTO. Thank you!

@TildaDares I have formatted it 😊.
Thanks @7malikk for your help

@7malikk
Copy link
Collaborator

7malikk commented Oct 15, 2022

@Tammy-Ajoko Nice work!
Could you make a tiny adjustment to the
sample code, it must've been a simple typo but you put let/cont instead of let/const.

Thank you!

@TildaDares
Copy link
Member

Hi @Tammy-Ajoko, great job on the formatting. Usually the diffs contains each line of code that needs to be changed. Looking at the files, we’ll need more than one line change.

@Tammy-Ajoko
Copy link
Contributor Author

@TildaDares ok I didn't know that before, but I have made the adjustments now

@TildaDares
Copy link
Member

@Tammy-Ajoko You also have to specify which keyword it should be exactly.

@Tammy-Ajoko
Copy link
Contributor Author

@TildaDares ok I have updated it again hope it is better now. Thank you for walking me through it, I'll definitely take correction when making another FTO

@TildaDares
Copy link
Member

It’s perfect. Thank you @Tammy-Ajoko!

@TildaDares
Copy link
Member

Reserved for @Favour-Cay

@NARUDESIGNS
Copy link

@TildaDares @Tammy-Ajoko
Great FTO, please also confirm that ALL those constant variables are not reassigned anywhere else in the code 👍

@TildaDares
Copy link
Member

Reserved for @Onyi-X for 24 hours!

@Onyi-X
Copy link

Onyi-X commented Oct 26, 2022

@TildaDares I'll like to work on it

@Onyi-X Onyi-X mentioned this issue Oct 26, 2022
5 tasks
@Egnodia
Copy link

Egnodia commented Nov 29, 2022

I'd like to try this!

@cesswairimu
Copy link
Collaborator

Hi @Egnodia, go ahead, please check if the file is still unchanged before you continue, Thanks

@Aarav238
Copy link
Contributor

Aarav238 commented Dec 2, 2022

Hey, I am new to this community and I want to contribute here,Please assign this issue to me. I would love to work on this.

@cesswairimu
Copy link
Collaborator

Hi @Aarav238, @Egnodia had expressed interest on working on this one, maybe you can checkout this one instead? #1174

@Egnodia
Copy link

Egnodia commented Dec 3, 2022

I dont understand , theres no 'var' to replace in the [pixelorigin.html] folder ? I'm confused

@Egnodia
Copy link

Egnodia commented Dec 3, 2022

I think the file is unchanged @cesswairimu

@AliJafriETH
Copy link
Contributor

Hey @cesswairimu, is the issue still open? I'd love to work on this :)

@cesswairimu
Copy link
Collaborator

Hi @Egnodia and @AliJafriETH, sorry for the late reply ;
Seems the js code was moved to a separate file https://github.com/publiclab/Leaflet.DistortableImage/blob/main/debug/pixelorigin.js - on this pull request #1216, some of the changes may already be done, if any of you would like to go through this file and see if there are any changes remaining please feel free to do so and create a pull request(please let the other person know you are working on it, before you get started).
Thanks you both

@AliJafriETH
Copy link
Contributor

Hey @cesswairimu, thank you so much for the update :))

I still see some changes that can be made, can i go ahead @cesswairimu & @Egnodia ?

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