-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Frontend: fix boolean field with null value display as null. #2523
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
Frontend: fix boolean field with null value display as null. #2523
Conversation
client/app/lib/value-format.js
Outdated
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.
Shouldn't we use empty string instead of 'null'
, like we do for numeric columns? And, BTW, here should be check for undefined
too (the same as below)
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 will fix that.
client/app/lib/value-format.js
Outdated
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 remove this empty line
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.
ok
d0725c8
to
138c081
Compare
138c081
to
ad0a981
Compare
@kravets-levko @arikfr sorting as defined in dynamic table is: // ASC -> DESC -> off note that the caret shows DESC order (I would expect to see the triangle flipped ^ ) looks like the order sign is flipped redash/client/app/components/dynamic-table/dynamic-table.html Lines 10 to 15 in 414faba
And the issue now with the ASC sorting is that it breaks: Order should be |
hi @innovia! it's because we use "raw" values for sorting. it's different issue (we already noticed it with number columns and null values), and it's not directly related to this PR. we'll fix it soon too, but for now let's merge your fix 😉 |
What about the caret direction? Seems like its fliiped |
cc: @kocsmy |
@arikfr please review |
On the nav it makes sense as it opens and close but it doesnt make sense on
a sorting of asc/desc
|
Oh sorry, I misunderstood this :) ASC: triangle pointing up, DESC: triangle pointing down. |
No description provided.