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

Replace container div with Fragment #223

Merged
merged 2 commits into from
May 18, 2022
Merged

Conversation

ajrussellaudio
Copy link
Contributor

A small change, but let me explain why we need it.

We have a div surrounding our ConsentManager component. That div applies styles, including a banner:

      <div className="border border-red">
        <ConsentManager
          writeKey={writeKey}

...which renders to the DOM as:

<div class="border border-mint">
  <div>
    <div class="css-h3yqvb">
      <div class="css-ow1vry">
        <p class="css-1u2wgn3">
          <!-- banner content -->
        </p>
// ...

If the user accepts cookies and the banner disappears, then the DOM is now:

<div class="border border-mint">
  <div></div>
</div>

i.e. our wrapper div, containing an empty div. The wrapper div is thus 0px * 0px, but the border is still visible, seen as a 2px square on the page.

This PR removes the empty div, replacing it with a React.Fragment. If none of the conditions are met in the container.tsx render, then only an empty Fragment is returned, which results in nothing being added to the DOM, and we can therefore hide the empty wrapper div at our end with some CSS:

div.myClass:empty {
  display: none;
}

Copy link
Contributor

@edsonjab edsonjab left a comment

Choose a reason for hiding this comment

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

The components shows correctly

@nd4p90x nd4p90x requested a review from pooyaj May 14, 2022 15:38
@pooyaj pooyaj merged commit a4633df into segmentio:master May 18, 2022
@ajrussellaudio
Copy link
Contributor Author

Awesome, thanks @edsonjab @nd4p90x and @pooyaj 👍

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.

4 participants