-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add missing job filters #645
Conversation
✅ Deploy Preview for ami-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
5082f05
to
ccc8b1f
Compare
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.
It works! It's great being able to link to jobs from the session detail / single capture. Thank you for that addition
@@ -14,13 +14,13 @@ export const EmptyState = () => { | |||
: STRING.MESSAGE_NO_RESULTS | |||
)} | |||
</span> | |||
{activeFilters.length && ( | |||
{activeFilters.length ? ( |
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.
@annavik I had to decide which of these changes to keep in the merge. Will you confirm that I chose the right one?
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.
Yes that looks right, thank you! Since activeFilters.length
is a number, not a boolean, the later solution is better. Before we actually had a 0
mistakenly rendered, instead of nothing in the case of no filters.
If you want to go down the React rabbit hole further here is a blog post explaining this little thing: https://www.codemzy.com/blog/react-render-0-conditional
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.
If you see a 0
out of context in any web app, it's probably because of this, it's a common mistake which I sometimes still do myself!! Another solution could be to cast the number to boolean !!activeFilters.length
, but I personally think the ternary makes code a bit more readable in this case.
@annavik I rebased main into this branch and the extra commits are now gone. It's actually a nice way to step through your changes! everything is working, so I am hoping I got it right, but will you double check? |
Thanks a lot for doing the rebase, looking good! You are right about the commits, that great. Even though we squashed them into to main, it's nice to keep the list clean for the PR history 👌 |
Note: needs rebase before merge! Base in not main.
Fixes #633 ! Also, I added some filter links from the session detail view, to make it possible to use the new filter
source_image_single
. I would like rework all bubble links at some point, to make it more clear what is going on here (not sure how many users understand what clicking these bubbles means), but for now I follow the same UI pattern as rest of the app.Screenshots