max-height/offsetHeight Calculation Issue, Fixes #2581

[terryaney]

Fixes #2581

If the number of items in the dropdown > this.options.size, then the following line runs:

menuHeight = liHeight * this.options.size + divLength * divHeight + menuPadding.vert;

However, liHeight is calculated wrong if the first item is hidden or if first item has text of ''.

Before adding the temporary newElement to the body to calculate height, I always show the first item (I didn't check to see if hidden), and if first items text = '', I set it to   (probably wouldn't need a check here either).

[NicolasCARPi]

Hello,

1. use single quotes, see: https://app.codacy.com/gh/snapappointments/bootstrap-select/pullRequest?prid=8039996
2. don't use variable name with a $. I know it's like that in some part of the code (not many), but I think it's weird and doesn't convey meaning (other than it's a jquery object)
3. please use long and meaningful variable names
4. itemIsBlank shouldn't exist, just compare directly
5. you should define $a first (renamed as per 3.) and call show() on it instead of defining it after and repeating the selector.

Note that I haven't run your code yet, this is a quick review on the diff I see.

[NicolasCARPi]

Hello Terry, thank you for your contribution, I made a quick review, please address my comments.

[terryaney]

1. Done
2. Yes, it was only to convey it was jquery object and I was copying your style from other place.
3. Basically a disposable variable, but chose newElementAnchor.
4. Bad code ;) Fixed.
5. Yes, missed that b/c I had a bit different code to start where I checked if hidden, then showed. Didn't clean up. Sorry.

[NicolasCARPi]

Use === here. I will add a proper eslint config at some point for this kind of stuff.

[terryaney]

Added === check and added some addition code to preserve/restore css. Let me know if anything else is needed.

[NicolasCARPi]

@terryaney thanks, please make the linter happy (you can run it locally, too!)

[terryaney]

@terryaney thanks, please make the linter happy (you can run it locally, too!)

I've updated. If linter still failing, could you tell me how I run locally? FYI, I'm editing the file in VS Code, but I'm not opening the folder or anything (just the single file). I'll google in the meantime.

[NicolasCARPi]

You can run it locally with npm run lint.

[terryaney]

Doesn’t seem to be working. Sent from Mail for Windows From: Nicolas CARPi Sent: Friday, January 14, 2022 3:25 PM To: snapappointments/bootstrap-select Cc: Terry Aney; Mention Subject: Re: [snapappointments/bootstrap-select] max-height/offsetHeight Calculation Issue, Fixes #2581 (#2653) You can run it locally with npm run lint. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: ***@***.***>

[NicolasCARPi]

Doesn’t seem to be working.

No it's working but you need to get a proper dev env with npm i.

[terryaney]

I guess I'm still confused because I expect there to be a "lint" script/action present inside package.json but it isn't there and I obviously don't want to edit that file. Disclaimer, I do 99% of my dev in Visual Studio on server side code so my npm/VS Code skills are still very green.

I ran npm i in the terminal and it said it added 451 packages (recalling from memory, so could be different) and then I tried npm run lint and recieved this:

0 info it worked if it ends with ok
1 verbose cli [
1 verbose cli   'C:\\Program Files\\nodejs\\node.exe',
1 verbose cli   'C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js',
1 verbose cli   'run',
1 verbose cli   'lint'
1 verbose cli ]
2 info using npm@6.14.8
3 info using node@v12.19.0
4 verbose stack Error: missing script: lint
4 verbose stack     at run (C:\Program Files\nodejs\node_modules\npm\lib\run-script.js:155:19)
4 verbose stack     at C:\Program Files\nodejs\node_modules\npm\lib\run-script.js:63:5
4 verbose stack     at C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:116:5
4 verbose stack     at C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:436:5
4 verbose stack     at checkBinReferences_ (C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:391:45)
4 verbose stack     at final (C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:434:3)
4 verbose stack     at then (C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:161:5)
4 verbose stack     at C:\Program Files\nodejs\node_modules\npm\node_modules\read-package-json\read-json.js:382:12
4 verbose stack     at C:\Program Files\nodejs\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:123:16
4 verbose stack     at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
5 verbose cwd C:\BTR\OpenSource\bootstrap-select
6 verbose Windows_NT 10.0.19043
7 verbose argv "C:\\Program Files\\nodejs\\node.exe" "C:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "run" "lint"
8 verbose node v12.19.0
9 verbose npm  v6.14.8
10 error missing script: lint
11 verbose exit [ 1, true ]

[NicolasCARPi]

@terryaney Sorry it's my bad, I mixed up projects in my head. This project uses grunt, so the command is grunt lint or ./node_modules/.bin/grunt lint if your $PATH is not properly set to use node's bins.

[terryaney]

OK, so to run it, it was simply ./node_modules/.bin/grunt (without the lint). When I ran it with lint, I got this warning Warning: Task "lint" not found. Use --force to continue.. But it seemed to work without the lint part and all my issues are resolved.

Secondly, after running that (or maybe the npm i command), I have 87 'dist' files that changed (I checked one and it seemed it was the copyright date) along with the package-lock.json file (that just updated version from 1.13.18 -> 1.14.0-beta2). I didn't check those in and just undid all those for now. Let me if anything else. Thanks for the guidance.

[NicolasCARPi]

See #2681 for the dist issue (no longer an issue).

You don't have the lint grunt command because you're not using the latest code. Make sure to use the dev branch for basing your PR.

[terryaney]

Going to start over after pulling latest code.