-
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
FIX: Histogram module selected value not displayed #1510
FIX: Histogram module selected value not displayed #1510
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1510 +/- ##
=======================================
Coverage 66.62% 66.62%
=======================================
Files 129 129
Lines 2661 2661
Branches 428 428
=======================================
Hits 1773 1773
Misses 888 888
|
@VladimirMikulic @harshkhandeparkar cause of the problem seems to be correct. But was just wondering, we also have a "select" type option in Edge-detect.In Edge-detect datatype is identified as a string but in Histogram, data-type is identified as Bool. Do you know why that happening? |
I'll have to check. I'll check later.
…On Wed, 15 Jan, 2020, 10:56 PM keshav234156, ***@***.***> wrote:
@VladimirMikulic <https://github.com/VladimirMikulic> @harshkhandeparkar
<https://github.com/HarshKhandeparkar> cause of the problem seems to be
correct. But was just wondering, we also have a "select" type option in
Edge-detect.In Edge-detect datatype is identified as a string but in
Histogram, data-type is identified as Bool. Do you know why that happening?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1510?email_source=notifications&email_token=AIJI5H6TLS3U2W5CAXPMMT3Q55BLLA5CNFSM4KHGIRV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJBDYKQ#issuecomment-574766122>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIJI5HYFUJV7M3QPAMCP23TQ55BLLANCNFSM4KHGIRVQ>
.
|
Error is because of this line |
Thanks @blurry-x-face. Yes, that is the error and I've updated the PR accordingly but I'll still keep my String constructor in defaultHtmlStepUi. It will serve like a security mechanism, if a programmer forgets to parse the value as a string, it will do it for him since everything in the DOM is a string. @keshav234156 this is ready, could you also approve this on GCI dashboard? Thanks. |
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!!
@VladimirMikulic Let's protect this with a UI test. Would you like to add a UI test for the test which check that all the modules the default options are displayed . I can publish a hard task related to this on the GCI dashboard if you want. |
@keshav234156 I can't promise anything, but if you publish it I'll see what I can do. |
@keshav234156 I'll be happy to write those UI tests! |
@blurry-x-face Ok, Go Ahead!! |
@jywarren it's ready for your final review!!! |
This is weird. I used to see this: And i still see that on #1453 Here I see this: And there's no option to Thanks! |
The histogram module values are "true" and "false". jQuery would coerce them to Boolean true and false instead of String "true" and "false" which would result in empty value being displayed. Resolves #1295
@jywarren now it should be fine. Thanks. |
I'm going to try to squash and merge. Maybe it just can tell it doesn't need to re-run checks? |
👍 |
Fixes #1295
The histogram module values are "true" and "false".
jQuery would coerce them to Boolean true and false instead of String
"true" and "false" which would result in empty value being displayed.
@keshav234156 could you review this? Thanks.