[PWA] rename components and move app specific code to /src #1490
Conversation
|
@pierreneter thanks so much for tackling this! This will take a significant amount of testing/review to be sure everything is working properly. I will start testing this out as soon as I have time. Once we've approved the changes, we can resolve the conflicts and get this merged. |
|
Ok started testing this out locally... The should respond with HTTP status 200
404 https://github.com/webpack/webpack.js.org/edit/master/src/content/support/index.md at build/support/index.html:19:31179 <a class="page-links__link" href="https://github.com/webpack/webpack.js.org/edit/master/src/content/support/index.md">...</a>which seems to be caused by links picking up the |
|
@skipjack This error occurred because I have moved the We should move it inwards or not? |
|
@pierreneter ah you're totally right, the edit link failures are to be expected.
No it's fine as is, this is to be expected in scenarios like this.
Can you carefully resolve all the conflicts? From there I'll do one more round of testing and make it a priority to get this merged. |
…A/RenameComponents
|
all done @skipjack |
| @@ -6,7 +6,7 @@ language: node_js | |||
| node_js: | |||
| - "6" | |||
| script: | |||
| - bash ./scripts/deploy.sh | |||
| - bash ./src/scripts/deploy.sh | |||
There was a problem hiding this comment.
We may want to keep the /scripts directory as is seeing as it's not part of the actual site code. @bebraw what do you think?
There was a problem hiding this comment.
Ok I'm going to leave this for now but may revert this change in a follow up PR. It's nothing major but I think it's good to separate build process related scripts from actual the actual javascript that runs the site (though the line is somewhat blurry for static site generators).
| import Link from '../link/link'; | ||
| import Container from '../container/container'; | ||
| import Link from '../Link/Link'; | ||
| import Container from '../Container/Container'; |
There was a problem hiding this comment.
Now that each file is named similarly just with a different extension, is there a potential for conflict here? Seeing as the site works, I'm guessing webpack tries .jsx files before .scss but isn't it somewhat ambiguous to which module this import goes? I do see that you did update all the styling imports to hit the .scss file directly. If this is common practice then I'm fine with it, just curious.
There was a problem hiding this comment.
Ok still interested in thoughts on this ambiguity but as far as I can tell this isn't causing any issues.
|
Aside from the |
These files are now placed in the same location under the `/src` directory.
It seems most of our content actually wasn't picked up by alex prior to the recent changes. This commit fixes/ignores some new warnings that cropped up.
|
@pierreneter did a final review, pushed some fixes, and will merge shortly. |
|
@skipjack thanks! |
|
@pierreneter no problem had to fix one thing in #1538 but aside from that everything seems 👍 (at least no "the site blew up" issues popped up so far 😆 ). Thanks again for your work on this, you may want to sync up with @hulkish in #1400 to discuss next steps. Note that I did check the first two items there off and added references to these PRs. It seems the other items there won't be as big overhauls but please feel free to ping me if I can help in anyway. I'm finding the PWA items, performance improvements, and lighthouse testing all very interesting. |
These files are generated by the `npm run fetch` script and were somehow messed up by #1490 (I think).
These files are generated by the `npm run fetch` script and were somehow messed up by #1490 (I think). Followed these instructions to ensure they were fully removed and ignored: https://stackoverflow.com/questions/11451535/gitignore-is-not-working Resolves #1554
Related to #1400
Finished the first two lines in this checklist.