First timer: Remove jQuery part 1 #540
Conversation
|
Thanks for opening this pull request! |
carlo-ev
changed the title
There was a problem hiding this comment.
@carlo-ev thank you for the PR! Sorry to get back to you so late - great work :) I left a review to complete before it's ready for merge
There was a problem hiding this comment.
Great work fixing the code after the first review. Now we also want to make sure cross domain requests work too.
Btw -- should have mentioned this earlier, we just started supporting ES6 if you prefer using the Fetch API. XHR is fine too though so totally your call!
There was a problem hiding this comment.
ok we're getting there thank you for sticking through it!
Some changes are
var request = new Request(open.exportStartUrl, requestOptions);I think you meant to doopts.exportStartUrl- in
_defaultFetchStatusUrland_defaultHandleStatusRes, you don't want to returnresponse.json(), I think insteadresponse.text(). (response.json()will throw an error).
You can scratch my above comment about rebasing it -- i figured out how to do it for you as you can see below in the commits
sashadev-sky
force-pushed
the
first-timer-jq-pt1
branch
3 times, most recently
from
41825b3 to
46766d1
Compare
|
@carlo-ev note before you make any changes you'l want to pul in locally the rebase i did for you here. -- so |
|
I fix the "opts" mistype and changed both response return from json to text. |
|
@carlo-ev no worries, just make sure you are checking your work in the future by running the code locally. Rebasing your dist file and then reviewing again! |
sashadev-sky
force-pushed
the
first-timer-jq-pt1
branch
2 times, most recently
from
2fe4ffb to
3ba0687
Compare
|
@jywarren @VladimirMikulic this looks good to me, any idea why it's getting stuck on the "compositing" step again? I think in the past that has indicated a CORS issue? I have it up on my gh pages here: https://sasha.mapknitter.org/examples/select.html |
|
@sashadev-sky could you clarify, where is it stuck? What action should be triggered? |
|
@VladimirMikulic i'm thinking since this was meant as an FTO and it's already implemented without async/await, we can save that for another PR. Are you on board with that? sorry should've gotten you involved earlier! Also did you try it out on the page i linked? It just continues compositing endlessly. Meaning something is going wrong but it's not being indicated in the console. |
This comment has been minimized.
This comment has been minimized.
|
@VladimirMikulic sorry that was my bad!! I turned on enforce https in my gh-pages settings. I'll make the change you requested because that was actually my suggestion not the contributors and request your review again for merge |
sashadev-sky
force-pushed
the
first-timer-jq-pt1
branch
from
3ba0687 to
31d6b55
Compare
|
That's what I suspected haha :) |
|
@VladimirMikulic I'll make a follow up FTO! |
|
@carlo-ev thank you so much for the great work! |
|
Congrats on merging your first pull request! 🙌🎉⚡️ |
* refactored two ajax request to simple xhr * added verify to request success * changed requests to fetch api * Rebase * check out package.json * Revert package.json state to main * change request url mistype and fixed response transform * Rebase * Rebase package-lock * Update response 200 to response.ok Co-authored-by: sashadev-sky <[email protected]>
|
@sashadev-sky thank you so much for the opportunity to help! Hope there's some more minor issues I can help with. |
|
@carlo-ev what are you interested in? |
|
@sashadev-sky maybe issues #338 or #311 if that's ok, saw a lot already claimed on the "help wanted". |
|
Yes, nobody is currently working on those so that would be awesome!! Thank you :) Feel free to let me know if you need any guidance on them! @carlo-ev |
Fixes #538
@sashadev-sky
Thanks for the opportunity to contribute for the first time, please let me know if I have to change something.
Thank you once more!