Skip to content

Bootstrap Upgrade #72

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Jun 20, 2023
Merged

Bootstrap Upgrade #72

merged 13 commits into from
Jun 20, 2023

Conversation

h4ckd0tm3
Copy link
Contributor

Oh boy... That was a journey but behold CTFPad is now running with the newest Bootstrap Version (5.3.0). It is possible that I missed some things somewhere but as for now I am pretty confident I change everything that needed to be changed in order for everything to work and look nice.

@hugsy
Copy link
Owner

hugsy commented Jun 12, 2023

We have different formatters for HTML. This makes every change from you re-format the code and then same for me. This is crazy annoying.

Before I review I will add various formatters (html, js , css, python). Please make sure your IDE follow them.

@h4ckd0tm3
Copy link
Contributor Author

We have different formatters for HTML. This makes every change from you re-format the code and then same for me. This is crazy annoying.

Before I review I will add various formatters (html, js , css, python). Please make sure your IDE follow them.

Thanks! Just ping me when you are done and I will do the propper formatting :)

@h4ckd0tm3 h4ckd0tm3 mentioned this pull request Jun 14, 2023
Closed
@hugsy
Copy link
Owner

hugsy commented Jun 14, 2023

It's on the works now, and it will be:

  • black for python code formatting
  • djhtml for html templates/js/css

All enforced by a pre-push policy. Those integrations work fine on VScode, check on your IDE and lmk. This will all be done in the merge of #71

Cheers

@h4ckd0tm3
Copy link
Contributor Author

h4ckd0tm3 commented Jun 14, 2023 •
edited
Loading

It's on the works now, and it will be:

  • black for python code formatting
  • djhtml for html templates/js/css

All enforced by a pre-push policy. Those integrations work fine on VScode, check on your IDE and lmk. This will all be done in the merge of #71

Cheers

Checked and should be fine with PyCharm. 👍

EDIT: Question tho: Shouldn't it be rather "pre-commit" than "pre-push"? Because if you check after commit the changes are already commited no? But if you check pre-commit the hooks would run before the commit is done and commit the correctly formatted files. At least that's my understanding of it.

@hugsy
Copy link
Owner

hugsy commented Jun 14, 2023 •
edited
Loading

It will be pre-push for the repo to prevent PRs that do not comply.
But locally, I encourage to format at each commit (TBH I format at every save), however I don't wanna control that, everyone has their own preference.

There might also be an action coming for this at some point to refuse PR merging if not compliant to black

@hugsy
Copy link
Owner

hugsy commented Jun 14, 2023

Pretty much everything is ready, I'll double-check it all today after work because it'll be quite a big change 😂

@h4ckd0tm3
Copy link
Contributor Author

Glad to see your changes merged! What's left for me to do now?

@hugsy
Copy link
Owner

hugsy commented Jun 19, 2023

Somehow this has made things worst 😭

image

I think I will just test your branch, if all works I'll merge and then fix everything manually so formatting stuff like that don't ever happen.

@hugsy
Copy link
Owner

hugsy commented Jun 19, 2023

This is not manageable, can you revert to before a24b5f8 and force push? I'll review from that point directly in GH and then merge. Just like this I have no idea what changes you made... 😔

I'll apply the formatting later on.

@h4ckd0tm3
Copy link
Contributor Author

Will do, just gimme a few min!

@h4ckd0tm3 h4ckd0tm3 force-pushed the bootstrap_upgrade branch from a24b5f8 to d906199 Compare June 19, 2023 15:53
hugsy
hugsy reviewed Jun 19, 2023
View reviewed changes
ctfpad/templates/ctfpad/ctfs/detail_challenges.html
@@ -75,57 +88,46 @@


<script>
function filterChallenges()
{
function filterChallenges() {
Copy link
Owner

Choose a reason for hiding this comment

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

Just realized, this now can go into /js/challenges.js but no that can be done in another PR

hugsy
hugsy reviewed Jun 19, 2023
View reviewed changes
ctfpad/templates/ctfpad/main.html
integrity="sha384-TX8t27EcRE3e/ihU7zmQxVncDAy5uIKz4rEkgIXeMed4M0jlfIDPvg6uqKI2xXr2" crossorigin="anonymous">
<link rel="stylesheet" type="text/css" href="{% static '/css/fontawesome/css/all.min.css' %}" />
<link rel="stylesheet" type="text/css" href="/static/css/main-{% theme_cookie %}.css" />
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.0/dist/css/bootstrap.min.css" rel="stylesheet"
Copy link
Owner

Choose a reason for hiding this comment

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

🎆

hugsy
hugsy reviewed Jun 19, 2023
View reviewed changes
ctfpad/templates/ctfpad/main.html
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.0/dist/css/bootstrap.min.css" rel="stylesheet"
integrity="sha384-9ndCyUaIbzAi2FUVXJi0CjmCapSmO7SnpJef0486qhLnuZ2cdeRhO02iuK6FUUVM" crossorigin="anonymous">
<link rel="stylesheet" type="text/css" href="{% static '/css/fontawesome/css/all.min.css' %}"/>
<link rel="stylesheet" type="text/css" href="/static/css/main.css"/>
<link rel="icon" type="image/png" sizes="16x16" href="{% static '/images/favicon.png' %}">
<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.9.4/Chart.min.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

TODO:
Chart.js can be bumped ? Latest is now 4.3

hugsy
hugsy reviewed Jun 19, 2023
View reviewed changes
hugsy
hugsy reviewed Jun 19, 2023
View reviewed changes
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

I reviewed everything, awesome stuff! So glad to get rid of the dark mode hack and jquery.

I will need to test it live because it looks like some HTML parts are duplicated. Minor stuff but ugly so I wanna be sure. If all good I'll merge then.

@h4ckd0tm3
Copy link
Contributor Author

Nice :) yeah could be that I missed some stuff. I made quite the changes and could be that some copy paste stuff stayed there.

@hugsy
Copy link
Owner

hugsy commented Jun 19, 2023

Nice :) yeah could be that I missed some stuff. I made quite the changes and could be that some copy paste stuff stayed there.

Yeah 🙂 I suspected so

@h4ckd0tm3
Copy link
Contributor Author

Alright, I think I fixed everything that was broken.
Tested every button in the UI to see if everything works, we really need tests xD

hugsy
hugsy approved these changes Jun 20, 2023
View reviewed changes
Copy link
Owner

@hugsy hugsy left a comment

Choose a reason for hiding this comment

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

Let's go

@hugsy hugsy merged commit aabe35a into hugsy:master Jun 20, 2023
@h4ckd0tm3 h4ckd0tm3 deleted the bootstrap_upgrade branch June 21, 2023 07:04
@hugsy hugsy added this to the 0.1 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugsy hugsy hugsy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@h4ckd0tm3 @hugsy