Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

allow render_react_component(name, props, class: "...")#9

Open
gfx wants to merge 2 commits into
airbnb:masterfrom
bitjourney:html_options
Open

allow render_react_component(name, props, class: "...")#9
gfx wants to merge 2 commits into
airbnb:masterfrom
bitjourney:html_options

Conversation

@gfx

@gfx gfx commented Dec 8, 2016 •
edited
Loading

Copy link
Copy Markdown

We need to specify HTML class attributes to the wrapper div elements.

Can you review it please?

BTW I can't pass the test (#8) so the CI will fail :(

ljharb
ljharb suggested changes Dec 8, 2016
View reviewed changes

@ljharb ljharb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate on why you need this? Specifically, why does any code besides hypernova's clientside JS need to reference this div?

Comment thread lib/hypernova/blank_renderer.rb Outdated
attributes = ''
options = job[:html_options]
if options && options[:class]
attributes << %{ class="#{options[:class]}"}

@ljharb ljharb Dec 8, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what if options[:class] includes a double quote? please include a test case that covers this.

@gfx gfx Dec 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

How about cb90ed4 ?

@gfx

gfx commented Dec 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

Can you elaborate on why you need this? Specifically, why does any code besides hypernova's clientside JS need to reference this div?

This is because we need to apply CSS.

Out code includes CSS something like this:

foo {
  *[data-hypernova-key] {
     // blah blah blah
  }
}

This works but I think it is ugly and I don't want to depend on hypernova-specific things in CSS. If this PR is merged, I just add a CSS selector to the hypernova's div.

@ljharb

ljharb commented Dec 8, 2016

Copy link
Copy Markdown
Collaborator

@gfx what CSS are you trying to apply that can't be applied to the root element of your react tree?

@gfx

gfx commented Dec 9, 2016

Copy link
Copy Markdown
Author

Here you are:

  *[data-hypernova-key] {
    height: 100%;
  }

The height of a block element must be specified to the very element.

@ljharb

ljharb commented Dec 9, 2016

Copy link
Copy Markdown
Collaborator

To clarify, does this mean you're rendering the entire HTML content with react, and you want it to take up the whole screen?

@juanca

juanca commented Jul 11, 2022 •
edited
Loading

Copy link
Copy Markdown

I have a couple of use-cases:

  1. Reduce unnecessary <div>s by styling the generated div instead of another wrapping <div>.
  2. Apply a grid area name to the container to make the generated div fit an area of the page (sub-grid feature is not yet supported)
  3. Apply a sticky position on either the generated div to make the react component sticky against the document.

Put it all together:

<body style="grid-template-areas: 'nav main'">
  <div style="grid-area: nav">
    <%= render_react_component('nav', { ... }, { style: "position: sticky; top: 0" }
  </div>
  <main>...</main>
  </div>
</body>

I could move the grid-area: nav to the generate div and make the react root component sticky. But either way, AFAIK, there is a necessity to style the generated div.

Unfortunately, I cannot render the whole application in React because our codebase is a mixture of accumulated tech stacks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ljharb ljharb ljharb requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gfx @ljharb @juanca