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

Update L.DistortableImageOverlay syntax in remaining tests #198

Closed
6 tasks
sashadev-sky opened this issue Apr 7, 2019 · 15 comments · Fixed by #234
Closed
6 tasks

Update L.DistortableImageOverlay syntax in remaining tests #198

sashadev-sky opened this issue Apr 7, 2019 · 15 comments · Fixed by #234

Comments

@sashadev-sky
Copy link
Member

First Time?

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 groups underrepresented in free and open source software!

We know that the process of creating a pull request is the biggest barrier 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.

The problem

What's supposed to happen:

From the official Leaflet docs:

Class Factories

You may have noticed that Leaflet objects are created without using the new keyword. This is achieved by complementing each class with a lowercase factory method:

new L.Map('map'); // becomes:
L.map('map');

We have recently added a factory method to the L.DistortableImageOverlay class and updated our code to initialize new instances of this class using it:

L.distortableImageOverlay = function(id, options) {
return new L.DistortableImageOverlay(id, options);
};

Since testing is a form of documentation in addition to a way of ensuring stable functionality, we want to keep our tests consistent with our code syntax. We'll need to update our test files.

Solution

Where to find the relevant lines of code:

Remove the new keyword and change L.DistortableImageOverlay to L.distortableImageOverlay in the following code blocks:

overlay = new L.DistortableImageOverlay('/examples/example.png', {
corners: [
new L.LatLng(41.7934, -87.6052),
new L.LatLng(41.7934, -87.5852),
new L.LatLng(41.7834, -87.5852),
new L.LatLng(41.7834, -87.6052)
]
}).addTo(map);

distortable = new L.DistortableImageOverlay('/examples/example.png', {
corners: [
new L.LatLng(41.7934, -87.6052),
new L.LatLng(41.7934, -87.5852),
new L.LatLng(41.7834, -87.5852),
new L.LatLng(41.7834, -87.6052)
]
}).addTo(map);

distortable = new L.DistortableImageOverlay('/examples/example.jpg', {
corners: [
new L.LatLng(41.7934, -87.6052),
new L.LatLng(41.7934, -87.5852),
new L.LatLng(41.7834, -87.5852),
new L.LatLng(41.7834, -87.6052)
]
}).addTo(map);

distortable = new L.DistortableImageOverlay('/examples/example.jpg', {
corners: [
new L.LatLng(41.7934, -87.6052),
new L.LatLng(41.7934, -87.5852),
new L.LatLng(41.7834, -87.5852),
new L.LatLng(41.7834, -87.6052)
]
}).addTo(map);

Thanks!!

Step by Step

  • Claim this issue with a comment here, below, and ask any clarifying questions you need
  • Fork the repository and set it up locally following the main repo README instructions https://github.com/publiclab/Leaflet.DistortableImage
  • Create a new feature branch with a unique name descriptive to the issue
  • Try to fix the issue following the steps above, but even before you're done, you can:
    commit your changes to your branch and start a pull request (see contributing to Public Lab software) but mark it as "in progress" if you have questions or if you haven't finished
  • Reference this issue in your pull request body
  • Once you submit your pull request, if there's an additional checklist provided for getting it merged, get those boxes checked off. Either way, mention me @sashadev-sky for a review.

Please keep us updated

💬⏰ - We encourage contributors to be respectful to the community and provide an update within a week of claiming a first-timers-only issue. We're happy to keep it assigned to you as long as you need if you update us, but if we don't see any activity a week after you claim it we may reassign it to give someone else a chance. Thank you in advance!

If this happens to you, don't sweat it! Grab another open issue.

💬 Get help

If you need any help - here are some options:

@Priyak5
Copy link
Contributor

Priyak5 commented Apr 8, 2019

@sashadev-sky heyy, Can I take this or it would be better for a new comer?
Thank you!

@sashadev-sky
Copy link
Member Author

@Priyak5 typically would say leave it for a newcomer but I know you were a little confused about the Git workflow the first time around so I think you can do another FTO :)

But, don’t start it yet!! I’m gonna change it up to maybe add some sort of educational git challenge. @gauravano I might message you for ideas.

I’ll ping you when it’s ready :)

@sashadev-sky
Copy link
Member Author

Ok @Priyak5 here are your instructions:

Just do #1 from above first!

  • make a mistake (but one that doesn't break the tests). Perhaps change new L.DistortableImageOverlay to new L.distortableImageOverlay.
  • commit and push your changes
  • you see your mistake on your remote PR, and you think oh, it's just easier to edit it remotely over here. So you click on the files changed tab. And you fix it to L.distortableImageOverlay remotely. - - Save your changes and don't leave the page until the save is confirmed (could take a minute or so)
  • Now you're branch remote is diverged from your branch locally. If you push any changes local to remote, it will overrwrite the change you just made. What you need to is pull in your remote changes to your local branch: git pull origin <branchName>
  • and the challenge is done!

Do #2, #3, #4 as you would regularly.

Any questions??

@gauravano feel free to add anything here! (gauravano knows all the git tricks)

@Priyak5
Copy link
Contributor

Priyak5 commented Apr 9, 2019

@sashadev-sky Okay. On it.
Thank you!

Priyak5 added a commit to Priyak5/Leaflet.DistortableImage that referenced this issue Apr 10, 2019
@Priyak5
Copy link
Contributor

Priyak5 commented Apr 10, 2019

@sashadev-sky in step 3 it says fixing it remotely, what does it mean? When I go to file changed and fix my mistake, I have 2 options either to commit or create a new PR. What should be done after that ?
Thank you for your help!
Sorry if its a silly doubt..

@sashadev-sky
Copy link
Member Author

@Priyak5 no you had the right idea!! Re-open that PR! You fix the mistake remotely, and then can commit it to that branch just like if you were doing it from your terminal. After that, you just need to pull the changes back to your terminal, because your local repo doesn't know that you just made this commit. You need to send the commit to it so it's up to date, so you wont have conflicts the next time you push. Makes sense?

DW! Not a silly doubt I still get confused all the time 😅it gets a lot easier the more you keep doing it and seeing what works

@grvsachdeva
Copy link
Member

grvsachdeva commented Apr 10, 2019

@gauravano feel free to add anything here! (gauravano knows all the git tricks)

I surely don't know all 😅, but know enough to do my tasks. I still mess up many times but yes @sashadev-sky this 👇

it gets a lot easier the more you keep doing it and seeing what works

is apt!

Now you're branch remote is diverged from your branch locally. If you push any changes local to remote, it will overrwrite the change you just made. What you need to is pull in your remote changes to your local branch: git pull origin

It will overwrite only if we do force push i.e., use git push -f origin branch-name. In normal cases(when you push using git push origin branch-name), you'll not be able to push the changes and will see the message asking you to pull the remote changes. So, yeah, you'll pull the changes ultimately and force push should be done only when required!!!
(Nearly a year ago, I learned this lesson about force-pushing - https://stackoverflow.com/questions/50526457/i-pushed-a-commit-on-a-remote-branch-using-a-sha-which-deleted-7-past-commits#comment88068080_50526457 🙈 )

Thanks!

@sashadev-sky
Copy link
Member Author

@gauravano I guess were all living on the edge with force pushes!

@sashadev-sky
Copy link
Member Author

Hi, @Priyak5! just checking if you've gotten stuck on this at all, or if I could help in any way? Thanks!

@Priyak5
Copy link
Contributor

Priyak5 commented Apr 17, 2019

@sashadev-sky sorry for the late reply! My laptop stopped working, just got it back. Ill complete this today. Sorry again

@sashadev-sky
Copy link
Member Author

@Priyak5 thank you for the update! Let me know if you need anything

@sashadev-sky
Copy link
Member Author

@Priyak5 Hi! We have some requests for FTOs so I am going to re-assign this since you haven't taken action on it in a while and you have also done an FTO before. Sorry for any inconvenience! Please check out the help wanted issues on this repo there is a lot to do :)

@sashadev-sky
Copy link
Member Author

Re-assigning to @UNnamed66

@UNnamed66
Copy link
Contributor

Re-assigning to @UNnamed66

Gotcha, I'll work on it!

@UNnamed66
Copy link
Contributor

@sashadev-sky Please review, thanks.

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

Successfully merging a pull request may close this issue.

4 participants