-
-
Notifications
You must be signed in to change notification settings - Fork 771
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
[WIP] UI Inventory #1755
[WIP] UI Inventory #1755
Conversation
app/retail/views.py
Outdated
@@ -939,3 +939,11 @@ def tokens(request): | |||
key = f"{network}_tokens" | |||
context[key] = Token.objects.filter(network=network, approved=True) | |||
return TemplateResponse(request, 'tokens_js.txt', context, content_type='text/javascript') | |||
|
|||
def ui(request): |
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.
E302 expected 2 blank lines, found 1
let me know when this isnt WIP anymore and ill review |
Codecov Report
@@ Coverage Diff @@
## master #1755 +/- ##
==========================================
- Coverage 28.58% 28.53% -0.05%
==========================================
Files 128 128
Lines 10066 10089 +23
Branches 1328 1334 +6
==========================================
+ Hits 2877 2879 +2
- Misses 7088 7109 +21
Partials 101 101
Continue to review full report at Codecov.
|
app/assets/v2/js/pages/tabs.js
Outdated
var last = null; | ||
|
||
while (buttons !== null) { | ||
function onClick(evt) { |
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.
Move function declaration to function body root. (no-inner-declarations)
Don't make functions within a loop. (no-loop-func)
app/assets/v2/js/pages/tabs.js
Outdated
while (buttons !== null) { | ||
function onClick(evt) { | ||
var width = evt.target.offsetWidth; | ||
var offset = evt.target.offsetLeft; |
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.
Expected blank line after variable declarations. (newline-after-var)
app/assets/v2/js/pages/tabs.js
Outdated
indicator.style.width = width + 'px'; | ||
indicator.style.transform = 'translateX(' + offset + 'px)'; | ||
|
||
var current = sections.querySelector('#' + evt.target.id); |
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.
Expected blank line after variable declarations. (newline-after-var)
@owocki Ready for review. |
app/app/urls.py
Outdated
@@ -178,6 +178,7 @@ | |||
# brochureware views | |||
url(r'^about/?', retail.views.about, name='about'), | |||
url(r'^mission/?', retail.views.mission, name='mission'), | |||
url(r'^ui/?', retail.views.ui, name='ui'), |
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.
Please use re_path
since we are trying to migrate completely from url
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.
^
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.
In addition to the requested changes, can you please include an addition to the CONTRIBUTING.md
document explaining any addition of visual assets should be included in the inventory?
{% endcomment %} | ||
{% load i18n static %} | ||
<!DOCTYPE html> | ||
<html lang="en"> |
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.
Can you wrap relevant copy with {% trans "" %}
?
app/app/urls.py
Outdated
@@ -178,6 +178,7 @@ | |||
# brochureware views | |||
url(r'^about/?', retail.views.about, name='about'), | |||
url(r'^mission/?', retail.views.mission, name='mission'), | |||
url(r'^ui/?', retail.views.ui, name='ui'), |
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.
^
@PixelantDesign @mbeacom @SaptakS This is the how the page looks at the moment |
@thelostone-mc |
@pinkiebell lol thats cause I took a full page screenshot and the animation kicks in only when you actually scroll :P Btw just synced up with @PixelantDesign
|
Can you checkout the new changes? |
@pinkiebell wohoo ^_^ @PixelantDesign Merge ready ? @SaptakS / @mbeacom If in case you guys merge it before I get to it -> we need to change to uri to |
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.
LGTM
I am just curious if giving an icons like here to show the code is more intuitive than clicking on the card. |
Fixes #1755
Improve the code viewer and give the user a hint.
Just a quick rebase and I added some more assets + style fixes. PS: changed path to /styleguide-alpha |
Looks good thank you @pinkiebell |
|
||
<div class="container-fluid p-5 assets"> | ||
<p class="h1">{% trans "Vector Graphics" %}</p> | ||
<img src="{% static "v2/images/quickstart-box.svg" %}"/> |
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 am not saying we need to do it here... but we'll probably want to modify the vast majority of the asset handling and repetitive entries here to simply loop over assets in images/
and the associated subs / context data. We'd drop the line count in this file down by quite a bit and we wouldn't have to manually add every new asset as they're added (I'd be willing to bet it won't be long until we're missing assets here).
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.
That would be very nice. I'm not familiar with Django, so I will dig into that today. 👍
@SaptakS meh it's just for a first pass -> we'll improve it in cycles I already bugged @pinkiebell to add the code snippets (even though I believe it wasn't part of the original task :P ) |
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.
LGTM !
#1738