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.

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

Completion Of $step helper method #833

Closed
7 tasks done
harshkhandeparkar opened this issue Mar 5, 2019 · 38 comments · Fixed by #710
Closed
7 tasks done

Completion Of $step helper method #833

harshkhandeparkar opened this issue Mar 5, 2019 · 38 comments · Fixed by #710
Labels
help wanted testing

Comments

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Mar 5, 2019 •
edited
Loading

scopeQuery

I have an open PR #710 which refactors the UI code by adding a new helper method called scopeQuery which returns a scoped jQuery object(here it is assigned to a variable called $step) which only searches elements inside a given scope(DOM element). This method itself is complete but needs a test and needs to be documented.

Tests

The helper method is awesome and can shorten the UI code significantly but it should have a test which checks if it works properly so that the UI code doesn't break. I don't have much experience with jasmine(which is used to test UI code). If someone can add the test for me then it would be really great.

Documentation

Such a useful helper method should definitely get the attention of the contributors. It should be documented in the CONTRIBUTING.md file. If someone can do that as well, it would be great!

I Need Help

I am very busy and I want to get my PR merged but it should not be incomplete. If some contributor(s) can help me with the testing and documentation then I would be really grateful. All the information about this method can be found in the PR #710. If you are going to contribute to the tests then you can merge my branch into yours.

Note: All the 3 PRs will be merged together.

Break Me Up

This issue consists of two parts i.e. documentation and testing. Those two things are different enough from each other to deserve a dedicated issue for each. If you would like to break this issue into two separate ones it that would be helpful as well. Alternatively you can also solve one or both of these issues without breaking them up. Just let us know down in the discussion (comments).

Separated Issues:
Resolved Issues
  • Testing
  • Documentation
Open PRs

Thank you!

Your help makes Public Lab better! We deeply appreciate your helping refine and improve this site.

To learn how to write really great issues, which increases the chances they'll be resolved, see:

https://publiclab.org/wiki/developers#Contributing+for+non-coders
@harshkhandeparkar
Copy link
Member Author

@jywarren @publiclab/is-reviewers new issue for testing and docs for $step now open!

@harshkhandeparkar
Copy link
Member Author

I guess the documentation part can be an fto.

This was referenced Mar 9, 2019
Merged
Closed
@harshkhandeparkar
Copy link
Member Author

@harshithpabbati if you want to split up both the issues or any one, please note that the documentation issue should be formatted into a first-timers-only and the testing part can be a normal issue. If you want to solve the testing part, you can do that but please leave the docs part to a newcomer. Thanks.

@harshkhandeparkar
Copy link
Member Author

@harshithpabbati? Are you going to break this issue up?

@harshithpabbati
Copy link

Sorry I am traveling back to my home. So i can't work at this time.

@harshkhandeparkar
Copy link
Member Author

Ok np.

@harshkhandeparkar
Copy link
Member Author

@mgroovyank would you like to work on any one(or both) part(s) of this issue?

@harshkhandeparkar
Copy link
Member Author

Documentation issue open #885

@harshkhandeparkar harshkhandeparkar mentioned this issue Mar 22, 2019
Merged
4 tasks
@harshkhandeparkar
Copy link
Member Author

@jywarren docs pr done #916.

@harshkhandeparkar harshkhandeparkar mentioned this issue Apr 9, 2019
Closed
@Divy123
Copy link
Member

Divy123 commented Apr 9, 2019

I would like to work upon writing the UI tests for it.

@harshkhandeparkar
Copy link
Member Author

Thanks! Do you need any information about it?

@Divy123
Copy link
Member

Divy123 commented Apr 9, 2019

I will be exploring a bit and then discuss here,🙂

@harshkhandeparkar
Copy link
Member Author

Ok.😊

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

@harshkhandeparkar there are some things that are breaking out the code maybe, can you have a look at them. It was a huge PR and congrats for the merge.

@jywarren maybe we can revert the changes here now, and once we can have the things fixed we can remerge .

@Divy123 Divy123 reopened this Jun 25, 2019
@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

  1. This is the error currently in the console:
    Screenshot from 2019-06-25 16-07-23

Also just one thing more that also due to $step method not available in intermediateHtmlui.js

2). Also after refreshing the page, the url is correct but the steps inside url are not getting loaded.

3). I figured it out and what's happening is inside, defaultHtmlStepUi.js:

if (step.linkElements[index].contains(imgthumbnail))
        step.linkElements[index].href = step.imgElement.src;
    }

at the time of checking the condition, step.linkElements[index] is unavailable, maybe some async stuff here.

@harshkhandeparkar maybe we can get this fixed here and @jywarren if we can have a revert so that we can have the code cleanup issue go on and once this gets fixed we can continue but again great work Harsh.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Right now, the inserStep button is not working and showing this error. Also, I am seeing this error, in the console on starting up the server .
You can have a look at the screenshot I sent above to have a better understanding.
Hope that helps

@harshkhandeparkar
Copy link
Member Author

I am pretty sure that $step is available in the intermediateHtmlStepUi.js. Maybe a recent commit broke this.

@harshkhandeparkar
Copy link
Member Author

Can you tell which line throws the not defined error by expanding it?

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

@harshkhandeparkar its line 273 of defaultHtmlStepUi.js :

if (step.linkElements[index].contains(imgthumbnail))
        step.linkElements[index].href = step.imgElement.src;
    }

and step method is not working in line 92 in IntermediateHtmlStepUi.js where its showing $step is not defined.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Actually, I was suggesting a revert so that I can work on things till then as I was to work upon the same functionality and I won't be able to work without it done. Is that ok to you?

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Since I am also going to work on UI, so it would be better if I work on the latest code.

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Actually, I don't have the fix now and working without the fix may lead to issues later .

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Ya sure!!
Thanks a lot @harshkhandeparkar

@jywarren
Copy link
Member

How complex is the fix? We haven't pushed this to the live demo yet... would it be something we can patch relatively quickly?

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019 •
edited
Loading

I am not sure on this. Maybe @harshkhandeparkar can help on this..
Also till that I am on halt as I also have to work on things built over that functionality.
So for the time being, I will discuss with aashna on the UI testing..

But I suggest a revert so that new works by other people can go on smoothly for the time being. @jywarren

@jywarren
Copy link
Member

jywarren commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Ok trying it now!!

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

I tried it and on reverting the complete intermediate file gave me the correct result.
@jywarren do you want me to send a PR for that?

@jywarren
Copy link
Member

jywarren commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Can you please explain a bit on the bump part?
I am totally unaware of that.

@jywarren
Copy link
Member

jywarren commented Jun 25, 2019 via email

@Divy123
Copy link
Member

Divy123 commented Jun 25, 2019

Yes and thanks a lot for this great information.

@jywarren jywarren mentioned this issue Jul 1, 2019
Merged
4 tasks
@harshkhandeparkar
Copy link
Member Author

I think this can be closed.
cc @Divy123 @jywarren

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Shorten UI code with new $step method
4 participants
@jywarren @Divy123 @harshkhandeparkar @harshithpabbati