-
Notifications
You must be signed in to change notification settings - Fork 210
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
fixes insert step functionality #1266
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1266 +/- ##
==========================================
+ Coverage 32.07% 32.26% +0.19%
==========================================
Files 107 107
Lines 1986 1971 -15
Branches 296 292 -4
==========================================
- Hits 637 636 -1
+ Misses 1349 1335 -14
|
@jywarren @harshkhandeparkar can you please review this PR!!! |
@harshkhandeparkar could you please tell the step to reproduce the above |
Add a few steps, insert any step and try collapsing the dropdowns. |
there might be some error here that is causing this behavior
This behaviour is shown in beta version as well.So don't think that it has been caused by this PR.There are lots of other error in examples/lib/defaultHtmlStepUi.js,Will open a issue for same |
ok. ty. |
@jywarren can you please review this PR!!! |
if (insertStepSelect.val() == 'none') return; | ||
|
||
var newStepName = insertStepSelect.val(); | ||
function insert(id, $step, newStepName) { |
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 is looking good -- do you think we could make the name here more specific? Like, insert what? Insert step, right?
Thank you!!!
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.
@jywarren was thinking the same but insertstep was already defined in line 77,so left as it is
Thanks for the review @vaibhavmatta have you tested this locally and confirmed the fix? Thanks all! |
@jywarren Yes!! Though I tested it for the first time, but it worked fine for me. THANKS |
Great, thanks all!!!!! |
Fixes #1261
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/is-reviewers
for help, in a comment belowIf 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
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!