-
Notifications
You must be signed in to change notification settings - Fork 3
feature: add slug for article feature #2793
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
base: main
Are you sure you want to change the base?
Conversation
OpenAPI ChangesShow/hide 11 changes: 0 error, 0 warning, 11 infoUnexpected changes? Ensure your branch is up-to-date with |
f4fbe54 to
f3561d4
Compare
5a6221a to
34b1879
Compare
badec28 to
a4b5e39
Compare
ChristopherChudzicki
left a comment
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.
Left a few comments that I think would simplify this.
Also, could you add a test for retreival by slug to views_test.py ?
articles/views.py
Outdated
| return super().destroy(request, *args, **kwargs) | ||
|
|
||
|
|
||
| class ArticleDetailByIdOrSlugAPIView(APIView): |
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.
Request: Rather than a new APIView, I think we should reuse the existing one in order to share queryset and permissions. Two options:
- Override
get_objectto accept id or slug - Add a new action
(1) seems simpler since fewer APIs... was thinking something like
def get_object(self):
"""
Override to allow lookup by either ID (numeric) or slug.
"""
queryset = self.filter_queryset(self.get_queryset())
identifier = self.kwargs.get("pk")
if str(identifier).isdigit():
filter_kwargs = {"pk": int(identifier)}
else:
filter_kwargs = {"slug": identifier}
obj = get_object_or_404(queryset, **filter_kwargs)
self.check_object_permissions(self.request, obj)
return objThere 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 have tried to use the existing but one issue was the id remains compulsory which is number and generated by DRF and instead of changing the existing because it can be used later on for any other feature I thought we can create new api. We are applying the same permissions on the this new api.
export interface ArticlesApiArticlesRetrieveRequest {
/**
* A unique integer value identifying this article.
* @type {number}
* @memberof ArticlesApiArticlesRetrieve
*/
readonly id: number
}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.
After discussion, this was moved into an @action in order to reuse queryset and permissions.
| } | ||
| } | ||
|
|
||
| export const slugify = (title: string) => { |
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 would lean toward slugifying on the backend... We usually use django's slugify util.
I noticed that the current slug field is also nullable. We could make it required and set it to the title always pre-publish:
from django.utils.text import slugify
class Article(TimestampedModel):
def save(self, *args, **kwargs):
previous = Article.objects.get(pk=self.pk) if self.pk else None
was_published = getattr(previous, "is_published", None)
if not was_published:
self.slug = slugify(self.title)
super().save(*args, **kwargs)
May also want to deal with duplicate slugs (that seems easier on the backend).
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 have incorporated the above code and removed slugify from frontend. we have created the slug field with unique constraint and slug will be created only once so why we need to deal with duplicate? btw I have dealth with duplicate check the logic now please
slug = models.SlugField(max_length=255, unique=True, blank=True, null=True)
I have added the unit tests |
fa18241 to
63c94a5
Compare
95ad0f2 to
9fdf692
Compare
ChristopherChudzicki
left a comment
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.
The slug changes look good 👍
Could you fix the linting issue and also either
- move image changes to separate PR (preferred)
- or add testing instructions for them (is there an issue?)
64c63fa to
142eabc
Compare
for more information, see https://pre-commit.ci
|
The slug changes look good 👍
Done
I have added the instructions check if those make sense |
ChristopherChudzicki
left a comment
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.
Noticed a couple issues with the image uploading.
Suggestion: Move the image-related changes to a separate branch, we can merge the slug stuff as-is.
In general, it would be good to do distinct features on distinct branches.
| const imageNaturalWidth = img.naturalWidth | ||
|
|
||
| // If the image can't expand beyond the container, disable wide/full | ||
| setCanExpand(imageNaturalWidth > containerWidth) |
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.
If the image can't expand beyond the container, disable wide/full
Comparing the current container width and the image width isn't a reliable way to achieve this goal since the container width depends on the screen size. I see you added an event listener for window resize events, but that doesn't entirely solve the problem:
- user uploads a small image on small screen and selects option to make it full width
- image node data now has
layout: "full". - window resizes
- controls UI now removes the the full/wide options, but node still says "full"
- changing the node data on resize wouldn't help. The node data should be static. It should not depend on user's screen size.
My suggestion would be: compare to a constant value, like 900px, not to the screen size. The available options (and especially the node data) should not depend on user's screen size.
900px is our medium breakpoint, though potentially we want a higher value—full width images should be higher res—but that can always be tweaked in future.
| alt={alt || caption} | ||
| layout={layout} | ||
| ref={imgRef} | ||
| className={`${!canExpand ? "img-contained" : ""}`} |
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.
We should not do className={${!canExpand ? "img-contained" : ""}}.
The display of an image should only depend on the node attrs, not on the non-node state.
This leads to some odd behavior right now where:
- Insert a small image
- It looks ok, good.
- Delete it
- Press undo
- Now the image looks huge. (I guess the value of
canExpandis wrong in this scenario).
The above would be avoided if the display is a function of only the node attributes.
Suggestion: Should this instead be based on node.layout === "default" ?
| <Image src={src} alt={alt || caption} layout={layout} /> | ||
| <Image | ||
| src={src} | ||
| alt={alt || caption} |
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.
A screenreader will read the alt text then the caption, so using the caption as a fallback for the alt text doesn't seem beneficial to me. It would just result in the caption being read twice.
I'd leave this as alt={alt}; if there's no alt text...well, at least it will read the caption.
(That said, at some point, we should figure out how to make alt text required.)
|
|
||
| window.addEventListener("resize", checkSize) | ||
| return () => window.removeEventListener("resize", checkSize) | ||
| }, [src]) |
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.
BTW: I'm not sure you need useEffect here. I think passing the relevant logic as an onLoad callback to the Image would be fine.
What are the relevant tickets?
N/A
Description (What does it do?)
This feature is about adding slug, Have added the backend slug field and frontend will convert the title into slug on its first published. We have introduced new api which will return the article detail based on slug or id of the article.
Screenshots (if appropriate):
Screen.Recording.2025-12-09.at.2.12.16.PM.mov
How can this be tested?
Additional Context