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

Diet app #672

Closed
wants to merge 6 commits into from
Closed

Diet app #672

wants to merge 6 commits into from

Conversation

GorunowMaxim
Copy link

Background

Changelog

Copy link

netlify bot commented May 23, 2024

👷 Deploy request for pr-fsd pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 9791e76

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the example! I'll review the code later, but for now, could you please update the screenshot to include the app itself, past the login screen? The main goal of these screenshots is for people to estimate the complexity of the application before they dive into the code, so it would be helpful to see the dashboard or other pages on the screenshot

@illright
Copy link
Member

As for the build failure, I think the file extension .PNG needs to be lowercased .png

@GorunowMaxim GorunowMaxim requested a review from illright June 4, 2024 15:45
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the patience, I finally got to look at the code. I also tried our upcoming linter on your project to see how it performs, but I noticed that you have some non-standard additions to FSD. Particularly, the concept of subpages and subentities. It's a bit problematic for the following reasons:

  1. It's non-standard FSD, so it needs to be prominently documented in the README as project knowledge. Otherwise people will come to this project expecting an FSD structure, but will see something unexpected and get lost.
  2. It's an addition to FSD that hurts one of its core benefits — low coupling of sliced. The whole point of slices is to be independent, so in traditional FSD, there's no such thing as a subslice that shares code with its parent slice. It should either just be inside of one slice, or they should be completely separate slices (maybe in a slice group) and cross-imports between them should be avoided.
    I would recommend combining all subslices into one slice, and if it gets too big, try to extract something independent from it.

I also noticed that the imports in your project sidestep the public API, for example, widgets/waterWidget/ui/WaterWidget.tsx imports shared/ui/tooltip/Tooltip.tsx, but reaching into shared/ui is prohibited.

Also features/dailyChallenge seems empty, it should probably be removed.

And as for the image, you reuploaded it, but it still doesn't show the app's UI, could you please make a screenshot of some page that is not the sign-in?

@illright
Copy link
Member

Closing this because it got stale

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

Successfully merging this pull request may close these issues.

2 participants