Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/lore wiki page #675
base: dev
Are you sure you want to change the base?
Feature/lore wiki page #675
Changes from all commits
aa4ac4f
17d65c5
612185f
1a3e879
99080f8
33d45af
64c9027
dea71b2
d5ee29d
c0f69d7
9f2d6e5
49257d6
d43b644
31443bf
da6d810
ff5aefb
a99bb65
d8192ac
ba0a763
6f6f595
f38a527
3bf1b3a
31d6537
f1e3c35
b16dc06
b89ea66
e2bfa3d
2f5da3d
60c48d5
94e8144
4d9e2a4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You should add a comment here explaining why having an empty slug means that the title doesn't consist of actual letters, as it's easy to be (initially) confused and think that the code author mistakenly added an error for
title
when a different field is checked 😅Check warning on line 210 in src/internal/forms.py
Codecov / codecov/patch
src/internal/forms.py#L192-L210
Check warning on line 212 in src/internal/forms.py
Codecov / codecov/patch
src/internal/forms.py#L212
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.
SlugField
uses a defaultmax_length
of 50 if not specified (see the docs onSlugField
), and so we should either expandslug
'smax_length
to the same value, or "cut off" all characters beyond 50 in the form.Also,
verbose_name
should always be added - even if there are no good Norwegian translations (just having "slug" as the translation would be fine, I guess) - to have consistency between all model fields.Check warning on line 325 in src/internal/models.py
Codecov / codecov/patch
src/internal/models.py#L325
Check warning on line 333 in src/internal/models.py
Codecov / codecov/patch
src/internal/models.py#L328-L333
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 think it would make the CSS code more robust if you replaced
#lore-burger-icon + h2
with a new class (or ID) placed on theh2
element. This is both because it's somewhat common to tweak exactly which heading level the heading has (e.g. replacingh2
withh1
orh3
), and because it's also common to slightly change the order of HTML elements - which would make it necessary to update the+
part of the selector.The same goes for some similar selectors below.
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 would be great if you could add a comment (both here and an extra one in the template) explaining that the exact order and "siblinghood" (i.e. that if e.g. an element was added between
#burger-input
and#lore-burger-icon
, the code would no longer work) of the burger elements are important for this CSS code to work - especially considering that the+
and~
selectors are fairly uncommon to use, at least in my experience.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.
You should add a comment explaining that this code relies on the fact that CKEditor sets this CSS for images that are aligned left or right 🙂 Also - perhaps above that - you should explain why this code is here in the first place, i.e. a quick summary of what it's trying to do and potentially why.