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

Uncrispify DetentionProfileForm #3196

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Nov 13, 2024

@stveit stveit self-assigned this Nov 13, 2024
Copy link

github-actions bot commented Nov 13, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 992 0 11.22s
✅ PYTHON ruff 987 0 0.1s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Nov 13, 2024

Test results

    9 files      9 suites   8m 21s ⏱️
2 138 tests 2 138 ✅ 0 💤 0 ❌
4 015 runs  4 015 ✅ 0 💤 0 ❌

Results for commit df681f4.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.40%. Comparing base (9c879be) to head (df681f4).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3196   +/-   ##
=======================================
  Coverage   60.39%   60.40%           
=======================================
  Files         605      605           
  Lines       43703    43698    -5     
  Branches       48       48           
=======================================
+ Hits        26394    26395    +1     
+ Misses      17297    17291    -6     
  Partials       12       12           

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

@stveit stveit force-pushed the uncrispify/DetentionProfileForm branch from 36d24ca to 921b179 Compare November 13, 2024 15:11
@stveit stveit changed the base branch from master to uncrispify/add-css-classes-support-to-formcheckbox November 13, 2024 15:14
@stveit stveit force-pushed the uncrispify/add-css-classes-support-to-formcheckbox branch from 98e934d to 46fd948 Compare November 14, 2024 11:33
@stveit stveit force-pushed the uncrispify/DetentionProfileForm branch from 921b179 to 75e00ea Compare November 14, 2024 11:34
Base automatically changed from uncrispify/add-css-classes-support-to-formcheckbox to master November 14, 2024 12:10
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

The HTML that is generated does look a bit different, just as an example this is the difference of the active checkbox:

Previous

<div id="div_id_active" class="ctrlHolder">
    <label for="id_active">
        Active
        <input type="checkbox" name="active" class="input-align" id="id_active">
    </label>
</div>

Now

<div id="div_id_active" class="ctrlHolder input-align">
    <label for="id_active">
        Active
       <input type="checkbox" name="active" id="id_active">
    </label>
</div>

But since it does not make a difference on how it actually looks I am happy with it

@stveit
Copy link
Contributor Author

stveit commented Nov 14, 2024

The HTML that is generated does look a bit different, just as an example this is the difference of the active checkbox:

Previous

<div id="div_id_active" class="ctrlHolder">
    <label for="id_active">
        Active
        <input type="checkbox" name="active" class="input-align" id="id_active">
    </label>
</div>

Now

<div id="div_id_active" class="ctrlHolder input-align">
    <label for="id_active">
        Active
       <input type="checkbox" name="active" id="id_active">
    </label>
</div>

But since it does not make a difference on how it actually looks I am happy with it

Well thats annoying, but I think I know the solution. Sadly the solution will probably invalidate #3199 :P

@johannaengland
Copy link
Contributor

I actually don't know if these css classes are actually necessary, because if I remove them it still looks the same to me
@podliashanyk what do you think?

@johannaengland
Copy link
Contributor

Alright, after a little deep dive I believe we don't need the css class input-align at all, because it only does anything in combination with the class checkboxinput and that one has been removed from the template for many many years - actually when we went over to crispy forms - the commit removing checkboxinput is this one: 43f861e6016fd051267d8450e53

So #3199 was not necessary in the end I believe and we can simply remove these css classes

@stveit stveit force-pushed the uncrispify/DetentionProfileForm branch from abdb98d to 7e89add Compare November 14, 2024 14:29
@stveit stveit force-pushed the uncrispify/DetentionProfileForm branch from 7e89add to 0f077b6 Compare November 15, 2024 11:51
@stveit stveit force-pushed the uncrispify/DetentionProfileForm branch from 0f077b6 to df681f4 Compare November 15, 2024 11:57
@stveit stveit merged commit 42c7d52 into master Nov 15, 2024
11 of 12 checks passed
@stveit stveit deleted the uncrispify/DetentionProfileForm branch November 15, 2024 12:06
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.

python/nav/web/arnold/forms.py:DetentionProfileForm
2 participants