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

Update examples #705

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

creator-woow
Copy link

@creator-woow creator-woow commented Aug 7, 2024

Copy link

netlify bot commented Aug 7, 2024

👷 Deploy request for pr-fsd pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit e071ae7

@illright
Copy link
Member

I noticed that there are errors when I run Steiger, the FSD linter, on this project. Could you fix them please?

@creator-woow
Copy link
Author

@illright Yeah I see, I'm gonna fix it

@creator-woow
Copy link
Author

creator-woow commented Aug 12, 2024

@illright Fixed. Only 4 left, the reason is I can't name pages folder according fsd while using nextjs 14+, so linter doesn't check my pages folder (pages-view in my case). As soon as builder sees pages folder it starts build like I use pages router (while I'm using app router)
Снимок экрана 2024-08-12 в 23 48 43

@creator-woow
Copy link
Author

@illright The solution is for pages router, I use latest app router, so it's not fit

@creator-woow
Copy link
Author

creator-woow commented Aug 13, 2024

@illright Below was my case solution (https://feature-sliced.design/docs/guides/tech/with-nextjs#app-router). I fixed all except these two. The reason is that they referenced only once, but while app scaling amount on their references will be enough, so I don't thing these are problem
Снимок экрана 2024-08-13 в 19 08 02

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.

These errors point to unnecessary slices. Your app looks quite simple, so it doesn't really need the layer of features (and possibly, doesn't even need entities). Consider trying to fit the app in just Shared, Pages, and App, the architecture will be simpler.

@creator-woow
Copy link
Author

creator-woow commented Aug 22, 2024

@illright If I remove unnecessary slices, will you merge the request?

@illright
Copy link
Member

These slices are indeed a blocker, but I may have some more comments after this refactor

@creator-woow
Copy link
Author

@illright Okay, no staiger errors had stayed

@creator-woow creator-woow requested a review from illright October 8, 2024 17:00
@creator-woow
Copy link
Author

@illright Pls either review & merge, or close pr, if you won't merge it

@illright
Copy link
Member

I will review it when I get through other higher-priority things and find the time. I'm intending to merge this PR, so I don't want to close it, but I can't give an estimate for when I can do the review.

If you'd like to speed up the process of getting this PR merged, I can suggest asking someone in our Telegram chat for review. Perhaps they could point out some improvements and you could implement them ahead of the final review

@creator-woow
Copy link
Author

I will review it when I get through other higher-priority things and find the time. I'm intending to merge this PR, so I don't want to close it, but I can't give an estimate for when I can do the review.

If you'd like to speed up the process of getting this PR merged, I can suggest asking someone in our Telegram chat for review. Perhaps they could point out some improvements and you could implement them ahead of the final review

Got it, thank you for fast reply

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