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

Issue-1126: Slice TaskDetailView more SRP like #1132

Closed
wants to merge 1 commit into from

Conversation

ChrisKulesza
Copy link

@ChrisKulesza ChrisKulesza commented Dec 3, 2024

Preparatory work for the processing of Issue-1126

  • move TaskDetailTranslation and translationData to separate TS file
  • move TaskDetailViewSidebar to separate TS file
  • move tasksDetails to TaskDetails
  • move templateSidebar to TaskDetailViewTemplateSidebar
  • move buttons to TaskDetailViewButtons

Motivation

  • Structure code more legibly
  • encapsulate technical differences between the individual components

Which issues does this pull request close?

none

[OPTIONAL] Give testing instructions to reviewers

@ChrisKulesza ChrisKulesza self-assigned this Dec 3, 2024
@ChrisKulesza ChrisKulesza requested a review from a team as a code owner December 3, 2024 10:44
@ChrisKulesza ChrisKulesza requested review from felixevers and DasProffi and removed request for a team December 3, 2024 10:44
Copy link
Member

@DasProffi DasProffi left a comment

Choose a reason for hiding this comment

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

The changes look good and improve the readability a lot (especially moving the subcomponents outside the TaskDetailView Component function into separate components). I am unsure whether using multiple files is the best solution in regard to finding the right component to change (due to similar naming, it is unclear which is the main file).

The previously used single file component design, makes sure only the main component is exported, while now every subcomponent is exported too and thus within the search space of the app, although its use is only inside the TaskDetailView.

Either 1) declare each subcomponent its own reusable component, or 2) follow the single file component design.

Changes for 1):

  • make the props of the component public with an export and name them after the component
  • use and declare individual translation within each component (in this case, I would see it as optional)

Changes for 2):
Move the subcomponents and Translations back inside the TaskDetailView.tsx and only export the TaskDetailView component.

Apart from that, fix the linting for the CI.

@ChrisKulesza
Copy link
Author

The changes look good and improve the readability a lot (especially moving the subcomponents outside the TaskDetailView Component function into separate components). I am unsure whether using multiple files is the best solution in regard to finding the right component to change (due to similar naming, it is unclear which is the main file).

The previously used single file component design, makes sure only the main component is exported, while now every subcomponent is exported too and thus within the search space of the app, although its use is only inside the TaskDetailView.

Either 1) declare each subcomponent its own reusable component, or 2) follow the single file component design.

Changes for 1):

  • make the props of the component public with an export and name them after the component
  • use and declare individual translation within each component (in this case, I would see it as optional)

Changes for 2): Move the subcomponents and Translations back inside the TaskDetailView.tsx and only export the TaskDetailView component.

Apart from that, fix the linting for the CI.

Personally, I am flexible regarding how the Helpwave application is structured. In my day-to-day work, I am accustomed to organizing by features as well as following the SRP approach. If the preference here at Helpwave is to handle this with a large component approach, I am happy to implement it that way.

I have adapted this because working with a 450+ line component was a little difficult.

Could you please provide a clear direction? Thank you.

@DasProffi
Copy link
Member

Personally, I am flexible regarding how the Helpwave application is structured. In my day-to-day work, I am accustomed to organizing by features as well as following the SRP approach. If the preference here at Helpwave is to handle this with a large component approach, I am happy to implement it that way.

I have adapted this because working with a 450+ line component was a little difficult.

Could you please provide a clear direction? Thank you.

Keep the subcomponents TaskDetailViewSidebar, TaskDetails, TaskDetailViewTemplateSidebar, TaskDetailViewButtons, but move them into a single file (approach 2 from my previous message). Only export the TaskDetailsView and have the TaskDetailTranslation also within that file.

@DasProffi
Copy link
Member

Will be done in #1143

@DasProffi DasProffi closed this Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants