-
Notifications
You must be signed in to change notification settings - Fork 1.9k
chore: Maintainance #2063
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
base: gh-pages
Are you sure you want to change the base?
chore: Maintainance #2063
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🚦 Lighthouse Results (Mobile & Desktop)
|
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.
This PR is big (59 files), alters files being moved, has diffs in the moved files, and the. There is no explanation for these changes.
The linked Jekyll tutorial does not recommend this restructuring, and there’s no explanation provided for why it’s being done.
Before I would approve a PR like this, it would need to be limited to the changes required to achieve the goal. So no extraneous diffs. I would also want a compelling reason for touching so many files in the first place. Otherwise it looks like rearranging the furniture for no reason, and Id be inclined to close the PR. |
@jonchurch Thanks for the review! This will really help us manage website assets more gracefully, and it’ll make things much smoother if we ever redesign the site or migrate to a new platform. It improves DX and avoids unnecessary root clutter. Kind of like furniture—you can only use it if it’s arranged properly. 🫠 |
This PR is quite big (59 files), and it touches both moved files and their diffs. But I’d also point out that if we ever do a migration, the diffs will end up being even bigger than this. I’d like to avoid that scenario, since larger diffs usually lead to more mistakes. |
See discussion #2004
Why this change?