[ENH] Add link to external participant level search to filters block #2988
Conversation
* First draft of search link * Add the correct query link * Adding styling to Link using existing components --------- Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com> Co-authored-by: rmanaem <rmanaem@protonmail.ch>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2988 +/- ##
==========================================
- Coverage 65.15% 65.12% -0.03%
==========================================
Files 380 381 +1
Lines 24487 24553 +66
Branches 878 878
==========================================
+ Hits 15954 15990 +36
- Misses 8533 8563 +30 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This looks pretty good. I think the placement here is a little confusing since it ends up between the number of datasets and the filters you select. Maybe we could put it below the filters or as its own little section in the left sidebar?
Also it would be good if it was its own component but it's okay if not, this section has CSS that is pretty tightly connected anyways so it is hard to make it reusable and there's not much to test for this component.
| .third-party { | ||
| margin: 10px 0; | ||
| } | ||
|
|
||
| .third-party .on-accordion-wrapper .keyword-accordion { | ||
| .accordion-title { | ||
| position: absolute; | ||
| left: 300px; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
One suggestion is this can be better if the CSS is specific to the component. It's a bit messy here since we're reusing some global styles that haven't been moved to their own component.
import styled from "@emotion/styled"
const NeurobagelSearchDiv = styled.div`
padding-bottom: 10px;
.on-accordion-wrapper .keyword-accordion {
.accordion-title {
position: absolute;
top: -20px;
left: 280px;
}
}
`There was a problem hiding this comment.
Thanks for the suggestion @nellh! We refactored things into a NeurobagelSearch component, applied the emotion-styling, and added Neurobagel to the link for clarity.
|
Test failure here is unrelated, looks like a bug with our MongoDB mock. Thanks for contributing! Do you have examples of the tests failing locally for you? Tests shouldn't depend on configuration outside of the dependencies. |
- removed global styling - imported in FiltersBlock
We thought about positioning and we think that it makes most sense on the right with the results because it's not a filter constraint (as the things on the left). I pushed a change with the new
let us know which option you prefer |
I think most failing tests have something to do with a missing openssl dependency on my machine. I just installed See failed tests herenot quite sure what is causing this, but let me know if you like me to try things out |
|
IMO it looks better below the filter tags than above, though neither feels quite natural. I suspect we'll need to live with some sense of it being bolted on until we get some designer time focused on the problem. I do wonder if an entry at the bottom of the filters list on the left might feel cleaner. Something like this:
(Just fiddled around in inspector, so the |
We first put it on the left as well. We decided against it for two reasons:
That said: we'll go with what you guys prefer, just let us know! I also think it looks much nicer with the logo, we'll add that! |
|
My feeling is that the right already has a lot going on, and injecting this disrupts the flow. As long as we don't think sticking at at the bottom of the left would make people never see it (and I hope the full-color log will help), I think it makes more sense there. We could add a little bit more vertical space before the link, to separate it more from the filters. |
|
Hey @effigies, we just had another chat about it. Our preference would be below the filters on the right, also because below the UI elements on the left it would be outside the viewport on most common screen sizes and even when scrolled into view we think it wouldn't be very noticeable / distinguishable. I mocked it up locally:
I don't think it has to have a logo/branding on it either if that's a concern, but we'd prefer on the right side for those reasons. Let me know what you all decide to go with and we'll contribute that. |
|
Okay, yeah, zoomed out that is under-advertised. Here's another thought:
I couldn't get the vertical alignment right, but I assume it would be possible. It would put it in the same area as "things you can do with this query". |
I think it would also be fine above the filter list on the left, the rationale being that you would look there while starting to search for something and it feels fairly natural to discover the link there even if the function is different from the existing controls. Putting it at the top also keeps it within the common viewports. I also like the most recent version from @effigies with things on the right side in the desktop view. |
Hey, yeah, I agree this looks nice! We had even played a little with the "link on the right" idea before but didn't quite figure out what to do with the "clear all" button when keyword-filters are selected. We can give that a try and see what it does.
OK, let us try to make both options and then we can take a pick. |
above the filters Co-authored-by: Arman Jahanpour <rmanaem@users.noreply.github.com> Co-authored-by: Alyssa Dai <alyssa.ydai@gmail.com>
|
Hey folks, we tried out both ideas. I'll call them "right option" and "left option" for simplicity. Right option
We played around with this one quite a bit and I think getting a nice looking inline alignment that works with the "clear all" button on or off would need more than just adding a self-contained component, and we'd probably have to change hierarchy of rendered elements in the Left option
We added a horizontal divider here to distinguish it from the flow below and aligned it with the dataset list on the right when no tags are selected. @nellh, I couldn't figure out how to import the scss variable for the color directly inside the component ("newspaper" is the name), maybe you have a tip? Between these two options, we prefer the one on the left. I think it looks cleaner and fits better with the rest of the UI and the existing flow. The right option might look better if we actually get it to sit correctly inline, but I think having it between the "clear all" button and the tags that the "clear all" button removes would be a bit confusing. Let me know what you think. |
|
This looks great, I think we should move forward with the version on the left. |
|
Hey @nellh, great. I agree, left looks nice. What changes do you want to have before we can merge this? Do you mind having the logo as a svg directly in there or do you prefer a path? |
Proposed changes:
third-partytoaccordion.title, i.e. the?bitSome other notes / things to discuss