Skip to content

docs(concepts) make islands.md examples run without errors#1614

Merged
marvinhagemeister merged 8 commits into
freshframework:mainfrom
rawlingsr:children-of-the-island
Aug 12, 2023
Merged

docs(concepts) make islands.md examples run without errors#1614
marvinhagemeister merged 8 commits into
freshframework:mainfrom
rawlingsr:children-of-the-island

Conversation

@rawlingsr

Copy link
Copy Markdown
Contributor

Addresses #1582

I've tried to break up my changes so that only the first one or two commits can be taken if they are the only ones that are helpful.

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making a PR! This is great!!

Left two minor nit comments because the first argument to components is usually called Props not Data. It's more of a convention not a hard rule, but that makes it easier for folks coming from other frameworks.

The correct type for children is ComponentChildren in Preact instead of VNode<Element>

Comment thread docs/latest/concepts/islands.md Outdated
Comment thread docs/latest/concepts/islands.md Outdated
@rawlingsr rawlingsr force-pushed the children-of-the-island branch from 49927f9 to 508bd76 Compare August 11, 2023 00:15
rawlingsr and others added 2 commits August 10, 2023 18:18
Co-authored-by: Marvin Hagemeister <marvinhagemeister50@gmail.com>
Co-authored-by: Marvin Hagemeister <marvinhagemeister50@gmail.com>
@rawlingsr

Copy link
Copy Markdown
Contributor Author

@marvinhagemeister Thank you for the changes, and for the feedback.

I wanted to make one other change, but I am not sure what the text should say instead of what it says now. There is a paragraph that talks about not needing to serialize children: https://github.com/setibeings/fresh/blob/eafe8a901a3c9496561cc6b0059882a44e649747/docs/latest/concepts/islands.md?plain=1#L105C1-L109C1

It uses the pronoun "we" which I think could trip somebody up if they aren't sure if they're reading about something that THEY should be able to tell by looking at the HTML returned by the server.

@marvinhagemeister

Copy link
Copy Markdown
Contributor

@setibeings Good catch. Looks like that is a copy & paste error from the blog post on my part. That section is meant to refer to Fresh the framework itself.

@rawlingsr

Copy link
Copy Markdown
Contributor Author

I'm not sure if this is the blog post, but I'll just plan on reading it today and take a crack at rephrasing the paragraph I mentioned.

https://deno.com/blog/intro-to-islands

@marvinhagemeister

Copy link
Copy Markdown
Contributor

@setibeings it references a section from this blog post https://deno.com/blog/fresh-1.2#passing-jsx-to-islands-and-nesting-islands-within-each-other

@marvinhagemeister

Copy link
Copy Markdown
Contributor

Looks like there is a formatting error. Running deno fmt locally will resolve these and make the CI green 👍

@rawlingsr rawlingsr marked this pull request as draft August 12, 2023 16:09
@rawlingsr

Copy link
Copy Markdown
Contributor Author

Converting to draft because I now want to add an HTML Example.

@rawlingsr rawlingsr marked this pull request as ready for review August 12, 2023 16:43
@rawlingsr

Copy link
Copy Markdown
Contributor Author

Take a look at what I wrote and let me know if it matches what you think when you get a moment. Thanks for bearing with me.

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for making the PR 💯

@marvinhagemeister marvinhagemeister enabled auto-merge (squash) August 12, 2023 19:33
@marvinhagemeister marvinhagemeister merged commit 7679e27 into freshframework:main Aug 12, 2023
@rawlingsr rawlingsr deleted the children-of-the-island branch August 12, 2023 21:17
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