-
Notifications
You must be signed in to change notification settings - Fork 167
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
fix: comment not showing on Featured Projects #773
Conversation
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.
Thanks 👍 LGTM 🌈
@aqsaaqeel Can you rebase this to current master branch? |
|
@aqsaaqeel Can you pull the latest master branch and then rebase? I see conflicts in |
Hello @aqsaaqeel nice work, but I really don't think the backend was missing a Now if we go to the backend code of @tuxology @srish I will release a PR that reloves this from the front end as it was just a missing key somewhere. |
Please this is what I mean @aqsaaqeel and @tuxology .
so we can pick the length of comments from there
Hence the backend had no issue. It was just for the front end to reference both differently according to the fields from the backend, and the issue will be resolved. |
@benndip Ok, this is a good observation! I guess I was too quick in approving the PR 😅 @aqsaaqeel Sorry, we will close this since this will be resolved via #799 Thanks for your efforts! |
No it's not an issue. The backend has an architecture.... it's the way the
Frontend was designed by the Project component. The Project component us
being used for two different structures of data so that is why the issue
came up. If we two different components to handle the two different
structures of projects and staff-picked projects, it will be okay. But the
conditional statement still works fine as it shows a little of SOLID
PRINCIPLES in React.
…On Thu, 5 Oct 2023, 16:47 Ravish Ahmad, ***@***.***> wrote:
@benndip <https://github.com/benndip> If the comments_count is not
returning the correct value then isn't that something we should look into
and resolve? Fixing it on the frontend with a conditional is merely a
bandage, and not a well-rounded solution.
What do you think @tuxology <https://github.com/tuxology> ?
—
Reply to this email directly, view it on GitHub
<#773 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMF2EGH5D2TUAMTZBIE2VHTX53I7TAVCNFSM6AAAAAA5RYTAR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBZGE3DSNZUGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I feel this is a consistency issue and we should fix it on the backend. Nevertheless, even if we are okay with this, why is |
Hello Ravish, I understand your concern, but I'm also a backend developer,
and I have tested well that there's no inconsistency with it. It's just a
code misunderstanding :). We could connect better and I'll show you what I
mean.
…On Thu, 5 Oct 2023, 19:16 Ravish Ahmad, ***@***.***> wrote:
I feel this is a consistency issue and we should fix it on the backend.
Nevertheless, even if we are okay with this, why is comments_count not
the correct value if comments array is showing the right set of comments.
Something is definitely not right on the backed side and should be
addressed.
—
Reply to this email directly, view it on GitHub
<#773 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMF2EGE2YV7GEYMPWDA3SVLX532RTAVCNFSM6AAAAAA5RYTAR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBZGQYTSNBTGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sure, let's connect. |
Are you available now @rak16 you can join here https://meet.google.com/uvn-dybq-gwp |
Summary
The Projects under Featured Projects list are showing comments count now.
Closes #85, Closes #22
Solution
The response from
staff-picks
API did not have acomments_count
field. I have added it under theserializers.py / ProjectSerializer
Screenshots
Requests / Responses
Request
GET
/api/projects/staff-picks/
Response