-
Notifications
You must be signed in to change notification settings - Fork 209
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
Dropdown 'Select a Module' fix #1352
Conversation
@publiclab/reviewers, please check |
Codecov Report
@@ Coverage Diff @@
## main #1352 +/- ##
=======================================
Coverage 66.57% 66.57%
=======================================
Files 125 125
Lines 2546 2546
Branches 397 397
=======================================
Hits 1695 1695
Misses 851 851 |
Do you think adding a few comments to the code would help? |
I am not a mentor sorry😅. I am just a student and reviewing others' PRs.
…On Wed, 11 Dec, 2019, 10:11 PM Sparks, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In examples/demo.js
<#1352 (comment)>
:
> @@ -8,7 +8,27 @@ var defaultHtmlSequencerUi = require('./lib/defaultHtmlSequencerUi.js'),
window.onload = function () {
sequencer = ImageSequencer();
- function refreshOptions() {
+ options = {
+ sortField: 'text',
+ openOnFocus: false,
+ onInitialize: function () {
+ var that = this;
So can you accept gci task now?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#1352?email_source=notifications&email_token=AIJI5HYYKN76XLJXDKLUSLDQYEJ5NA5CNFSM4JZI6PO2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCO25CRY#discussion_r356709349>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIJI5HZ2UCJFBWYGQEIMCPTQYEJ5NANCNFSM4JZI6POQ>
.
|
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.
LGTM 👍! Well done!
@keshav234156 , can you accept my GCI task? |
examples/demo.js
Outdated
this.$control.on("click", () => { | ||
this.ignoreFocusOpen = true; | ||
setTimeout(() => { | ||
this.ignoreFocusOpen = false; // trigger onFocus and open dropdown |
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.
Sorry but I forgot to tell you that comments need to follow certain rules from now on. We want to increase the consistency of docs in the codebase. Please start with a Capital letter and end with a full stop.
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.
Sure.
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.
Is it okey now?
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.
Please add full stops. Sorry for delaying this.
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.
Like this? Sorry I’m not sure that understood word “full stop” correctly
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.
Awesome. Thanks a lot!
Awesome work! Thank you! |
* Update demo.js * Codeclimate * Update demo.js * Code comments * Comments codestyle * Update demo.js
Fixes #1307
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!