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

Created ChapterDetails Component #544

Merged
merged 12 commits into from
Jan 24, 2025
Merged

Conversation

Rajgupta36
Copy link
Collaborator

Resolves #474

Tasks:

  • Create ChapterDetails Component
  • Add Test Cases for ChapterDetails
  • Add Missing Test Cases for ProjectDetails
  • Fix Minor Issues

Media
Screenshot 2025-01-19 025950
Screenshot 2025-01-19 030008

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Some comments ⬇️

frontend/src/pages/ChapterDetails.tsx Outdated Show resolved Hide resolved
frontend/src/pages/ChapterDetails.tsx Show resolved Hide resolved
@Rajgupta36 Rajgupta36 requested a review from kasya January 20, 2025 18:12
@kasya
Copy link
Collaborator

kasya commented Jan 23, 2025

@Rajgupta36 sorry it took me a moment to get back to this one! Could you resolve conflicts? I'll review it as soon as that's ready to view!

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

It seems we need to be smarter here in a long-term perspective. How about creating a generic entity details page component (like the Card we have for projects/chapters/committees) that could be reused for any entity depending on the fields passed to the function. So here and for committee details page too we'd just use the same code.

@Rajgupta36 does it make sense?

@Rajgupta36
Copy link
Collaborator Author

Rajgupta36 commented Jan 23, 2025

It seems we need to be smarter here in a long-term perspective. How about creating a generic entity details page component (like the Card we have for projects/chapters/committees) that could be reused for any entity depending on the fields passed to the function. So here and for committee details page too we'd just use the same code.

We could use a generic component, but putting all logic into a single file might make things more complex. Would it be better to make it modular? Perhaps we can use two generic components and handle some features like the map (ChapterPage) and issues/releases (ProjectPage) in their respective pages. What do you think, @arkid15r and @kasya ?

@Rajgupta36
Copy link
Collaborator Author

Should I do this in this PR, @arkid15r?

@arkid15r
Copy link
Collaborator

Should I do this in this PR, @arkid15r?

Up to you, we can merge this one but it'll require more refactoring then.
Sorry I haven't suggested this approach earlier.

@Rajgupta36
Copy link
Collaborator Author

Should I do this in this PR, @arkid15r?

Up to you, we can merge this one but it'll require more refactoring then. Sorry I haven't suggested this approach earlier.

Okay, I completed this in a few minutes and created an issue (like the test cases earlier).

@Rajgupta36
Copy link
Collaborator Author

Also, @arkid15r, what about these issues: #536, #549, #550, #551 ? I think the refactor depends on these issues.

@Rajgupta36
Copy link
Collaborator Author

@kasya It’s ready for review.

@arkid15r
Copy link
Collaborator

Also, @arkid15r, what about these issues: #536, #549, #550, #551 ? I think the refactor depends on these issues.

The only one with map seems significant. And other don't look like they being taken care of.

Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

I'm going to merge it as is -- we're going to unify it from the code reuse perspective.

Thanks for adding this @Rajgupta36!

@arkid15r arkid15r enabled auto-merge January 24, 2025 01:55
@arkid15r arkid15r added this pull request to the merge queue Jan 24, 2025
Merged via the queue into OWASP:main with commit 708726e Jan 24, 2025
13 checks passed
@Rajgupta36 Rajgupta36 deleted the chapterdetails_comp branch January 24, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement chapter details page layout
3 participants