-
Notifications
You must be signed in to change notification settings - Fork 96
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
fix: add development environment variable #214
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #214 +/- ##
=======================================
Coverage 96.46% 96.46%
=======================================
Files 195 195
Lines 1839 1839
Branches 322 322
=======================================
Hits 1774 1774
Misses 60 60
Partials 5 5 ☔ View full report in Codecov by Sentry. |
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.
Great, thanks!
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.
Just one thing, as commented above.
7228c97
to
126ae2a
Compare
@arbrandes I just tried this locally with I also got the frontend-app-learner-dashboard/src/containers/WidgetContainers/AppWrapper/index.jsx Line 9 in 82ff0d7
to be if (false) { I'm doing a bit more digging now |
I think #218 should do the trick |
@brian-smith-tcril thanks for digging into this, Brian! I'll take a look at your PR |
To follow up on this, I tried frontend-app-learner-dashboard/src/containers/WidgetContainers/AppWrapper/index.jsx Line 9 in 82ff0d7
and saw I'm not sure what magic I did on Friday to get strings to parse as bools, but it doesn't seem to be working now (all non-empty strings are being treated as true) When I set |
I'll be bringing this PR our next team refinement session for prioritization. |
Hey @brian-smith-tcril and @arbrandes, Team Aperture was originally assigned |
@jsnwesson I think reviewing #218 first makes sense. If that solution is seen as problematic for some reason then this PR should be merged as a workaround until a better solution is agreed upon. |
#218 was merged. Do we still need this? |
nope! #218 should do the trick! |
Thanks, everyone! I'll go ahead and close this. |
Resolves #210