Skip to content
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

Uncrispyfy foundation5 field #3000

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Sep 20, 2024

Introduces a reusable field template for #2794 (see 550de56).

Found templates/foundation-5/field.html hidden gem, which is implicitly (and explicitly) used in forms across the app.
This PR as-is fixes:

Best reviewed: commit-for-commit.

@podliashanyk podliashanyk requested a review from a team September 20, 2024 12:57
@podliashanyk podliashanyk self-assigned this Sep 20, 2024
Copy link

github-actions bot commented Sep 20, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 988 0 23.18s
✅ PYTHON flake8 988 0 427.8s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Sep 20, 2024

Test results

    9 files      9 suites   8m 31s ⏱️
3 333 tests 3 333 ✅ 0 💤 0 ❌
6 405 runs  6 405 ✅ 0 💤 0 ❌

Results for commit e07a0a0.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.76%. Comparing base (46c253f) to head (e07a0a0).
Report is 102 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3000      +/-   ##
==========================================
+ Coverage   56.60%   56.76%   +0.16%     
==========================================
  Files         602      603       +1     
  Lines       43722    43720       -2     
  Branches       48       48              
==========================================
+ Hits        24747    24816      +69     
+ Misses      18963    18892      -71     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@podliashanyk podliashanyk force-pushed the uncrispyfy/foundation5-field branch 2 times, most recently from 67ec912 to 3ffff83 Compare September 23, 2024 07:17
@lunkwill42
Copy link
Member

Just an initial comment before I actually review: Yes, crispy-forms-foundation is mostly based on regular Django templates. NAV is currently (in principle) locked to crispy-forms-foundation==0.7.1, whose base templates can be found here: https://github.com/sveetch/crispy-forms-foundation/tree/0.7.1/crispy_forms_foundation/templates/foundation-5

You've discovered some of NAV's own overrides to those templates :)

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Essentially, we would just be re-implementing our own custom subset of crispy-forms if we go down this route, and that might be good enough.

The important part of it is that we shouldn't have to write complete form templates over and over, when crispy-forms is essentially just about re-using the same template over and over.

python/nav/web/templates/foundation-5/field.html Outdated Show resolved Hide resolved
python/nav/web/templates/foundation-5/field.html Outdated Show resolved Hide resolved
This template is extensively used across the whole site. 
Also removes redundant checkbox class append which is not defined anywhere in CSS and is never rendered in computed styles in browser.
@podliashanyk podliashanyk requested review from lunkwill42 and a team September 23, 2024 11:11
@podliashanyk podliashanyk marked this pull request as ready for review September 23, 2024 11:13
Comment on lines +8 to +9
# Copied from
# https://github.com/django-crispy-forms/django-crispy-forms/blob/1.8.1/crispy_forms/templatetags/crispy_forms_field.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so trivial that I wouldn't bother with citing a source...

@hmpf hmpf self-requested a review September 25, 2024 08:22
@podliashanyk podliashanyk merged commit 5d58d78 into master Sep 25, 2024
12 of 13 checks passed
@podliashanyk podliashanyk deleted the uncrispyfy/foundation5-field branch September 25, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants