-
Notifications
You must be signed in to change notification settings - Fork 370
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
feat: Favorite queries #520
feat: Favorite queries #520
Conversation
Issue explorerhq#119 Heart at top of query view allows user to toggle favorite or not Added Favorites list to top menu
Issue explorerhq#119 fix isort and flake8 issues
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
==========================================
+ Coverage 96.55% 96.60% +0.04%
==========================================
Files 52 54 +2
Lines 2238 2300 +62
==========================================
+ Hits 2161 2222 +61
- Misses 77 78 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@marksweb let me know if you have any questions or tweaks to this |
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.
This is one where I need to think a little differently to ignore the difference in US spelling 😂
I'll install this as it is to get a feel for it, but it looks good.
There's just a few comments I've left that should improve things a little.
Co-authored-by: Mark Walker <theshow@gmail.com>
Issue explorerhq#119 Incorporate review feedback
Thanks @lawson89! |
Co-authored-by: Mark Walker <theshow@gmail.com>
Issue explorerhq#119 Make favorite icon red to standout
Issue explorerhq#119 Make favorite icon a tag Reuse on front page of query list
@marksweb please take a look and let me know what you think. I'm not sure tbh why the unit-tests-future-versions failed? |
@lawson89 will take a look as soon as possible. That test has failed because Django's main branch has moved on to v5 after they released the 4.2 alpha. And v5 needs a minimum of python 3.10, so I need to update the test suite. |
@lawson89 I've updated the test suite for django 4.1 alpha and django 5 (main). Could you rebase your branch please? I can't update from here. |
@marksweb done. Not sure why precommit fails, flake8 passes for me? |
@lawson89 It's likely parts of the project that aren't part of this PR. The CI bot will check everything and there's a good chance I've added pre-commit but not yet ran it against the existing codebase. I'll take a look and there might be another update to master that needs rebasing soon. |
@lawson89 Ok I've updated master with pre-commit having fixed everything in it's checks. Rebase your branch and all will pass 👍 |
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.
This is looking good. Very nearly ready I think.
I've just added a few comments that I think improve things and complete the feature.
Issue explorerhq#119 Changes from code review
# Conflicts: # explorer/tests/test_views.py
@lawson89 Thanks for all your work on this! |
Allow users to 'favorite' queries
Issue #119