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 example apps #81

Merged
merged 7 commits into from
May 24, 2019
Merged

Update example apps #81

merged 7 commits into from
May 24, 2019

Conversation

damassi
Copy link
Member

@damassi damassi commented May 22, 2019

Adds a few more examples to the repo:

  • Basic: Excludes all SSR functionality. How one would use the lib in an everyday client-side app. (I've updated the main README with simple and SSR examples so the different emphases come across.)
  • Server-side Rendering: Bare-bones SSR example.
  • Gatsby: For folks using Gatsby, figured -- why not?
  • Kitchen Sink: Renamed the old /example folder. I think we should try to steer people to the the basic and bare bones SSR examples first and then the kitchen sink example after. For people who are already familiar with the lib this is the perfect reference doc for whatever they may need but I worry about less advanced users becoming overwhelmed / confused.

Todo

  • Once Make interactions optional #82 is merged, publish new version of fresnel without interactions requirement and then update examples with pointers to lib rather than ../ relative references to code -- babel + ts doesn't like it.

(As an aside, I fell in a serious babel / compiler worm hole trying to figure out ^... When using babel-node, or node -r @babel/register, it's not easy to compile code below the root location where process.cwd() lives (where node booted up). Things just don't want to work and there is a lot of ambiguity around how to address the issue online... Easier to steer clear 😓 and install from the package when needing to compile stuff server-side.)

@damassi damassi requested a review from pepopowitz May 22, 2019 06:10
@damassi damassi force-pushed the update-example-apps branch 2 times, most recently from 5b36d40 to f18b716 Compare May 22, 2019 06:20
@damassi damassi force-pushed the update-example-apps branch 4 times, most recently from 19d0cb5 to 4cd823e Compare May 22, 2019 06:54
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
examples/basic/README.md Outdated Show resolved Hide resolved
examples/basic/src/server.js Outdated Show resolved Hide resolved
examples/basic/src/server.js Outdated Show resolved Hide resolved
examples/basic/src/server.js Outdated Show resolved Hide resolved
examples/gatsby/src/html.js Outdated Show resolved Hide resolved
@alloy
Copy link
Collaborator

alloy commented May 22, 2019

Excellent stuff, @damassi! 👏 🙌

(As an aside, I fell in a serious babel / compiler worm hole trying to figure out ^... When using babel-node, or node -r @babel/register, it's not easy to compile code below the root location where process.cwd() lives (where node booted up). Things just don't want to work and there is a lot of ambiguity around how to address the issue online... Easier to steer clear 😓 and install from the package when needing to compile stuff server-side.)

Perhaps this should be using yarn workspaces? Because I do think examples should use local code, not published out-of-sync [with local lib] code.

@damassi
Copy link
Member Author

damassi commented May 22, 2019

Perhaps this should be using yarn workspaces

Agree; can set this up in a follow-up PR.

Copy link
Contributor

@pepopowitz pepopowitz left a comment

Choose a reason for hiding this comment

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

🙏 These are great!!! They helped me understand the usage a lot.

I left a few very minor suggestions.

examples/basic/src/server.js Outdated Show resolved Hide resolved
examples/gatsby/README.md Show resolved Hide resolved
examples/gatsby/src/pages/index.tsx Outdated Show resolved Hide resolved
examples/gatsby/src/pages/index.tsx Outdated Show resolved Hide resolved
@damassi damassi force-pushed the update-example-apps branch 3 times, most recently from 3d909a2 to efad9ab Compare May 22, 2019 23:15
@damassi damassi force-pushed the update-example-apps branch from efad9ab to 297ed93 Compare May 22, 2019 23:20
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@alloy
Copy link
Collaborator

alloy commented May 23, 2019

There’s a broken link here /~https://github.com/artsy/react-responsive-media/pull/81/files#diff-04c6e90faac2675aa89e2176d2eec7d8R317

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@damassi damassi force-pushed the update-example-apps branch from 6300976 to 4a792ef Compare May 23, 2019 21:13
@damassi damassi force-pushed the update-example-apps branch from 4a792ef to 252ba42 Compare May 23, 2019 21:16
@damassi
Copy link
Member Author

damassi commented May 23, 2019

Alright, one more look when y'all get a sec 😄 Addressed remainder of feedback.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@alloy
Copy link
Collaborator

alloy commented May 24, 2019

Almost there!!

Additionally, I noticed this sentence in the README:

However, this approach suffers from a few flaws when used in conjunction with server-side rendering (SSR)

And would like to replace ‘flaws’ with something less loaded. Perhaps something like:

However, this approach has some limitations for what we wanted to get out of server-side rendering:

?

@alloy
Copy link
Collaborator

alloy commented May 24, 2019

I took the liberty to address my own feedback, so that I could start the rename work.

@alloy alloy merged commit 362320e into master May 24, 2019
@alloy alloy deleted the update-example-apps branch May 24, 2019 12:06
@damassi
Copy link
Member Author

damassi commented May 24, 2019

Thanks for the review everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants