-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
✨ Feature flag grid and package of the week + psa #1053
Conversation
|
||
{% switch "switch_use_grid_of_the_week" %} | ||
<div class="container"> | ||
<div class="row"> | ||
{% include "homepage/grid_of_the_week_partial.html" %} | ||
{% include "homepage/django_package_of_the_week_partial.html" %} | ||
{% switch "switch_use_psa" %} | ||
{% include "homepage/public_service_announcement_partial.html" %} | ||
{% endswitch %} | ||
</div> | ||
</div> | ||
</div> | ||
{% endswitch %} |
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.
I really like this. I haven't had the opportunity to use waffle
before, but this makes me want to try it out 😄
@@ -43,7 +43,7 @@ | |||
|
|||
def test_homepage(db, tp, django_assert_num_queries): | |||
url = tp.reverse("home") | |||
with django_assert_num_queries(10): | |||
with django_assert_num_queries(15): |
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.
@jefftriplett is the increase in expected number of queries because of the introduction of the use of waffle?
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.
I believe so. There's an option to use redis (I haven't read it in a while) but that's kind of a moot point when testing it. We can test with the flags off and on to narrow it down, but I was mostly looking for a way to ship your feature before dependabot starts burying us in updates :)
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.
Looks good 👍 we could add some documentation on where and how we use it if this is not a temporary thing.
<div class="row"> | ||
{% include "homepage/grid_of_the_week_partial.html" %} | ||
{% include "homepage/django_package_of_the_week_partial.html" %} | ||
{% switch "switch_use_psa" %} |
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.
Does this mean we need both this and "switch_use_grid_of_the_week" switch on to make PSA visible?
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, exactly. We'll probably launch something for it before the PSAs. I'll update that text block to point to our mastodon since that's low-hanging fruit.
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.
Btw, these auto-register in the admin so there isn't a guessing game to find them.
The PR adds a few feature switches around the Gride of the Week + Package of the Week. The PSF flag is primarily a place holder until we swap out some copy.