-
Notifications
You must be signed in to change notification settings - Fork 25
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 directory structure documentation #948
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.
LGTM 👍
I won't move in to "Reviewer approved", so that other team members could take a look ad say their objections, of they have.
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.
LGTM with some small comments.
Should we use this opportunity to remove the ### CLI Development
part from this doc? We don't officially support that stuff yet, so it seems confusing to include it in the docs.
├── preview-site/ # Preview sites feature | ||
│ ├── components/ # Feature-specific components | ||
│ ├── hooks/ # Feature-specific hooks | ||
│ └── lib/ # Feature-specific utilities |
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.
How do we deal with module specific Redux stores..? Have we discussed that?
I would keep everything in the src/stores
folder, and I guess you could argue that this makes sense because Redux state is inherently global.
So, I'm OK with the structure as currently proposed, but still wanted to bring this up for discussion.
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.
We decided to have only global Redux stores.
It says it's only for development, so I don't see anything against keeping it there. |
Related issues
Proposed Changes
Testing Instructions
Pre-merge Checklist