-
Notifications
You must be signed in to change notification settings - Fork 70
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
Frontend Improvements #85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
=======================================
- Coverage 80.31% 79.31% -1%
=======================================
Files 22 22
Lines 1011 1054 +43
=======================================
+ Hits 812 836 +24
- Misses 199 218 +19
Continue to review full report at Codecov.
|
@@ -33,6 +33,7 @@ MANIFEST | |||
# Static files | |||
polaris/polaris/collectstatic/* | |||
!polaris/polaris/collectstatic/__init__.py | |||
polaris/polaris/static/base.css |
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 file is now created via python manage.py compilescss
so we don't want to check it into version control
@@ -29,6 +29,9 @@ django-cors-headers = "*" | |||
toml = "*" | |||
pyjwt = "*" | |||
django-sslserver = "*" | |||
libsass = "*" | |||
django-compressor = "*" | |||
django-sass-processor = "*" |
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.
Packages needed to compile SCSS
@@ -74,7 +74,7 @@ def after_deposit(cls, transaction: Transaction): | |||
@classmethod | |||
def form_for_transaction( | |||
cls, transaction: Transaction | |||
) -> Optional[Type[forms.Form]]: | |||
) -> Optional[Tuple[Type[forms.Form], Dict]]: |
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.
Is there any way to make this Dict a class so we can get intellisense in vscode or something?
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.
not a big deal at all more just curious about python
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.
We could do something like this:
class PolarisFormContext:
def __init__(self, form: forms.Form, context: Dict):
self.form = form
self.context = context
Or we can remove self.context
and just make self.title
, self.guidance
, self.icon_label
, etc.
But personally I'd rather not require the user to use this object. Logically its the same, but if you think the mutli-return-value-or-none thing weird we can do 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.
no its not weird, i was thinking more about how to autocomplete what values of Dict are allowed. Definitely not something we need for this PR
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.
Oh I see. Any keys that aren't used are simply ignored
<link rel="stylesheet" href="{% sass_src 'base.scss' %}" /> | ||
<script | ||
async | ||
src="https://unpkg.com/flatpickr@4.6.3/dist/flatpickr.min.js" |
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.
Is it possible to bundle these resources up so we don't have to make so many network requests?
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.
Thats probably a question for Carl. I can't seem to be able to tag him though
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.
The autocomplete results are restricted to repository collaborators and any other participants on the thread.
But you can still do @vcarl
Major Changes:
form_for_transaction()
return type