Expose null sort controls to user. #325
Conversation
jmcarp
force-pushed
the
feature/expose-sort-null
branch
from
bdd1ed2
to
c0749b2
Compare
| @@ -60,7 +60,7 @@ before_script: | |||
| - cd openFEC | |||
| - git checkout $FEC_BRANCH | |||
| - psql -c 'create database cfdm_test' -U postgres | |||
| - psql -f data/subset.sql cfdm_test -U postgres | |||
| - pg_restore --dbname cfdm_test data/subset.dump --no-acl --no-owner | |||
There was a problem hiding this comment.
Is there a reason this was changed in this review?
There was a problem hiding this comment.
Yep, we deleted a bunch of old code from the API recently, which included the old subset. This code uses the current subset.
|
This looks good from a code perspective, will wait on @noahmanger @LindsayYoung for UI review. |
|
I think it is important for the user to understand that nulls are being filtered out. Otherwise, the sort will change the count and things will seem to appear and disappear. (I talked about this with Josh, but wanted to document it here) |
|
Sorry, just looking at this now. This seems really tricky. Important, but tricky. First question I have is whether it might make more sense to have this in the box above the table where the pagination controls are. It's not really a filter in the same way the others are filters because it always changes depending on which column you're sorting. Once we move the "Show" / "Hide Filters" button from that box too we'll have more space. The second question is around language. What about simply "Hide results with missing values when sorting" ? Or something along those lines? |
|
@noahmanger both of those changes sound good to me. I'll update the patch unless folks want to discuss more. @LindsayYoung does this sound good to you? |
|
+1 to "Hide results with missing values when sorting"
|
|
OK, I'm putting the revised null sort control right above the table. LMK if anything else should change before merge. |
|
Oh sorry, it should be in that same box with the pagination controls. But otherwise, looks good! |
|
OK, moved. @noahmanger or @msecret, feel free to continue fiddling with layout. This may also be good enough to merge and update with future PRs. |
|
Looks good to me. Yeah, I think we can move it once the new filter sidebar is merged. Looks like the tests ran into a snag, so I just restarted to see if that helps. |
|
Huh. It's throwing this error: |
|
Blah. I'll look at the tests and update here when they're fixed. The error probably isn't related to this patch. |
…/expose-sort-null
Expose null sort controls to user.
Instead of automatically hiding rows with
NULLvalues on sorted columns, allow users to control whether nulls are hidden or not. Any improvements on language / look and feel / default behavior welcome @LindsayYoung @noahmanger.