-
Notifications
You must be signed in to change notification settings - Fork 80
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
Listing tags #2092
Listing tags #2092
Conversation
This should be ready for review after #2088 is merged. This code has been deployed in the test env and seems to be working fine. Note that I had to update some js and css libraries Example of functionality, see how the "regular" search and the tags interact nicely: |
self.assertEqual(response.body, exp) | ||
|
||
|
||
class TestStudyTags(OauthTestingBase): |
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.
should there be negative test cases?
Should I not review this then? |
@antgonza can you pull from upstream/master? |
Changes Unknown when pulling fdb6723 on antgonza:listing-tags into ** on biocore:master**. |
@wasade, @josenavas, @ElDeveloper, could you review? |
Thanks @antgonza code looks good! Answering your last question - I think either looks fine, I don't have a strong preference. What I don't really like is the position of the filter. i.e. I don't like it showing inside the table. Is it easy to have on top of the table (i.e. above the headers?). The reason why I don't like it there is because it looks like only applies to the top table, but it should be applied to both tables (private and public studies). Do you think that change can be done easily? |
Option 2 looks great 👍 |
My vote goes to 2 - I like when things are modular and can be moved around!!! :D |
Changes Unknown when pulling e306f32 on antgonza:listing-tags into ** on biocore:master**. |
merge? |
|
ToDo: