Added second spinner #691
Added second spinner #691
Conversation
|
@publiclab/is-reviewers |
| @@ -216,11 +217,16 @@ function DefaultHtmlStepUi(_sequencer, options) { | |||
| function onDraw(step) { | |||
| $(step.ui.querySelector(".load")).show(); | |||
There was a problem hiding this comment.
One thing that I'd like to propose here is storing all these "query selected elements" into their respective descriptively named variables and using them instead so as to make the code more readable. Thanks!
There was a problem hiding this comment.
Yes that seems good. Also I was thinking something. step.ui.querySelector is used a lot of times in this code so instead maybe we can write a shorthand function which returns step.ui.querySelector wrapped in jquery.
Maybe
function stepSelector(queryString){
return $(step.ui.querySelector(queryString));
}If this idea is good then it can be a new issue even. Maybe an fto-candidate.
There was a problem hiding this comment.
Definitely @harshkhandeparkar! Thinking about it now makes me wonder that there must be many more snippets such as this one in the codebase that can be optimized. We can actually open up and pin an issue with a label like first-timer-opportunity, promoting among new contributors the practice to find and execute such refactor opportunities hidden in the codebase to make it more readable and submit those refactored codes as fto PRs which can help us immensely in the future!! Thoughts?
There was a problem hiding this comment.
Thats an amazing idea. Maybe we can even add an fto issue to create another issue with many such refactor opportunities and then mark that issue as break-me-up and that issue can then be broken up into many more ftos. We can also create one issue about this stepSelector now and give it as an example. Thoughts?
There was a problem hiding this comment.
One thing that I'd like to propose here is storing all these "query selected elements" into their respective descriptively named variables and using them instead so as to make the code more readable. Thanks!
I appreciate your idea, and think that refactoring can be done for the entire project as you guys are discussing. And I think let's keep refactoring separate from this pr. Also if there are any more suggestions , please provide inputs.
There was a problem hiding this comment.
@jywarren We'd love to hear from you regarding the above ideas. Thanks!
|
I guess this is good to go |
|
Thanks! |
|
Just a few notes here-- I think jQuery has toggleClass(classname) if that
helps.
I love the refactoring idea too and agree it could be addressed in a
multiple use FTO - perhaps see some examples of this in the "support" issue
label in plots2, where we did something like that during GCI.
Thanks everyone!
…On Mon, Jan 21, 2019, 12:36 PM Pranshu Srivastava ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/lib/defaultHtmlStepUi.js
<#691 (comment)>
:
> @@ -216,11 +217,16 @@ function DefaultHtmlStepUi(_sequencer, options) {
function onDraw(step) {
$(step.ui.querySelector(".load")).show();
@jywarren <https://github.com/jywarren> We'd love to hear from you
regarding the above ideas. Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#691 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ2yDBGsoNNUGobAWsmGugXRjbQHWks5vFfp6gaJpZM4aJUvK>
.
|
|
Is this ready for final review? :-)
…On Mon, Jan 21, 2019, 2:13 PM Jeffrey Warren ***@***.*** wrote:
Just a few notes here-- I think jQuery has toggleClass(classname) if that
helps.
I love the refactoring idea too and agree it could be addressed in a
multiple use FTO - perhaps see some examples of this in the "support" issue
label in plots2, where we did something like that during GCI.
Thanks everyone!
On Mon, Jan 21, 2019, 12:36 PM Pranshu Srivastava <
***@***.*** wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In examples/lib/defaultHtmlStepUi.js
> <#691 (comment)>
> :
>
> > @@ -216,11 +217,16 @@ function DefaultHtmlStepUi(_sequencer, options) {
> function onDraw(step) {
> $(step.ui.querySelector(".load")).show();
>
> @jywarren <https://github.com/jywarren> We'd love to hear from you
> regarding the above ideas. Thanks!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#691 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABfJ2yDBGsoNNUGobAWsmGugXRjbQHWks5vFfp6gaJpZM4aJUvK>
> .
>
|
|
@jywarren yes it is ready for final review. All the PRs with the label are ready for final review. |
|
Ok! Ready to merge as soon as it's rebased, thank you!!! |
|
@jywarren I have rebased, it's now ready to merge. |
|
Super great work!!! |
Fixes #679
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/is-reviewersfor help, in a comment belowThanks!