Skip to content
This repository has been archived by the owner on Jun 27, 2024. It is now read-only.

feat(render): dynamic head content #213

Merged
merged 8 commits into from
Sep 4, 2023
Merged

feat(render): dynamic head content #213

merged 8 commits into from
Sep 4, 2023

Conversation

jaymanmdev
Copy link
Contributor

@jaymanmdev jaymanmdev commented Sep 4, 2023

update the JSX render class to not explicitly inject HTML inside of the body tag, allowing for head meta tags and such to be injected and handled by the browser.

…he body tag, allowing for head meta tags and such to be injected and handled by the browser.
boywithkeyboard
boywithkeyboard previously approved these changes Sep 4, 2023
Copy link
Collaborator

@boywithkeyboard boywithkeyboard left a comment

Choose a reason for hiding this comment

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

lgtm

@boywithkeyboard
Copy link
Collaborator

@jaymanmdev seems like the test suite failed because of the changes you made. Please read the test file and try to fix the issue.

@boywithkeyboard
Copy link
Collaborator

Other than that, I like the idea behind this pr! :)

…ightly change render body assignment to not inject empty style tag if twind isn't utilised.
@jaymanmdev
Copy link
Contributor Author

jaymanmdev commented Sep 4, 2023

Sorry, this is my first ever PR in any repo so I am still getting the hang of it, I'm getting in the habit of running the tests before submitting an adjustment to my PR/s from now on. Thanks!

Edit: I can merge the two render tests together later on if that would be preferred. :)

@boywithkeyboard
Copy link
Collaborator

image

Our contributing guidelines are really basic but that's the one thing that's included in them. 😅

@jaymanmdev
Copy link
Contributor Author

image

Our contributing guidelines are really basic but that's the one thing that's included in them. 😅

Ah, I didn't pick up on that guidelines file - my bad! I tested the suite this time though.

@boywithkeyboard
Copy link
Collaborator

Just make a sub-test, e.g.

Deno.test('something', async (t) => {
  await t.step('first step', async () => {
    // some code
  })
})

@jaymanmdev
Copy link
Contributor Author

Just make a sub-test, e.g.

Deno.test('something', async (t) => {
  await t.step('first step', async () => {
    // some code
  })
})

I'll quickly do this now, and submit an adjustment.

…ut Twind styling to test for empty style tag injection, and 1 to test for meta tag injection in head.)
render.ts Outdated Show resolved Hide resolved
render.ts Outdated Show resolved Hide resolved
@not-ivy not-ivy changed the title update the JSX render class to not explicitly inject HTML inside of t… feat: allow dynamic head content Sep 4, 2023
render.ts Outdated Show resolved Hide resolved
…d of slicing the HEAD_TAG constant. (changes requested by not-ivy)
… css from Twind straight away, and return the html as is without any changes if that is the case)
not-ivy
not-ivy previously approved these changes Sep 4, 2023
… or body tags to ensure that the browser and document is still generated appropriately, remove the head manipulation in the render function as the browser will usually know where to put specific tags upon request.
@jaymanmdev
Copy link
Contributor Author

b134105

Let me know what you think.

  1. I removed the head manipulation that I was doing as the browser knows where to put specific tags, unless specifically stated otherwise (e.g. putting a meta tag inside the body tag.). We can build out better sanitization of the HTML later on if you'd like?
  2. I added a new test case to test this.

@boywithkeyboard boywithkeyboard changed the title feat: allow dynamic head content feat(render): allow dynamic head content Sep 4, 2023
@boywithkeyboard boywithkeyboard changed the title feat(render): allow dynamic head content feat(render): dynamic head content Sep 4, 2023
Copy link
Collaborator

@boywithkeyboard boywithkeyboard left a comment

Choose a reason for hiding this comment

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

lgtm

@boywithkeyboard boywithkeyboard merged commit d70410f into boywithkeyboard-archive:dev Sep 4, 2023
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants