-
Notifications
You must be signed in to change notification settings - Fork 6
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
💄 [#1309] Make upload widget correspond with design #580
💄 [#1309] Make upload widget correspond with design #580
Conversation
9f12f73
to
e61ddb7
Compare
fe4ef13
to
c6dc362
Compare
Codecov Report
@@ Coverage Diff @@
## develop #580 +/- ##
========================================
Coverage 96.17% 96.17%
========================================
Files 564 564
Lines 19795 19798 +3
========================================
+ Hits 19037 19040 +3
Misses 758 758
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
0700f91
to
204d608
Compare
src/open_inwoner/components/templates/components/Form/FileInput.html
Outdated
Show resolved
Hide resolved
{% load i18n form_tags button_tags icon_tags %} | ||
|
||
<div class="form__control {{ extra_classes|default:"upload" }}"> | ||
<div id="inputfile-group" class="inputfile-group"> |
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.
Should we maybe use a different name for id
and class
?
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.
@vaszig I've done a bit of research on naming conventions, since we don't have one standard yet in this project.
I found it's not technically necessary, but 'good practice' to have different names for ID's and classes in HTML.
Plus: I will set ID-names with an underscore from now on, and classes with a dash (because for classes the dash can be used as a modifier). I was using camelCase for some ID names, because of javascript, but that's not useful for CSS.
src/open_inwoner/js/components/upload-document/file-input-errors.js
Outdated
Show resolved
Hide resolved
behavior: 'smooth', | ||
}) | ||
documentUpload.classList.add('upload--open') | ||
for (let i = 0, max = getFormInfo.length; i < max; i++) { |
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't we use only variable "i" here?Why do we need "max" as well?
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.
My guess is to save the limit condition (because .length
is a calculated property)
But for-i
loops are terrible, it is much better preferred to use .forEach()
on the queryselector result, something like:
getFormInfo.forEach(elem => { elem.style.display = 'none'; })
(a for-of
loop would be nice but I don't know if that works with every weird JS itearble thing)
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.
Thanks @Bartvaderkin and @vaszig - I shall attempt to improve this (amongst all the other things).
src/open_inwoner/js/components/upload-document/show-file-info.js
Outdated
Show resolved
Hide resolved
77cc019
to
5f1724c
Compare
0ef4dee
to
01546ec
Compare
@vaszig and @Bartvaderkin : I have rebased this one to develop so it works with the CMS. |
Discussed, can be reviewed again by @vaszig . We might need to revisit setting the style display=none in show-file-info.js, but this can be double-checked on the testenvironment after merging |
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.
It seems really nice!I don't see any problems concerning cps headers in my local environment, so we have to make sure we don't have any on the other environments as well.
issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/1309
multiple tasks:
other task:
@requests_mock.Mocker() /class TestCaseDetailView(ClearCachesMixin, WebTest):
'