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.

Sign up for GitHub

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

Jump to bottom

Helix styles are dependent on spacing #283

Closed
mcortez1497 opened this issue Jul 11, 2018 · 8 comments
Closed

Helix styles are dependent on spacing #283

mcortez1497 opened this issue Jul 11, 2018 · 8 comments
Labels
CSS

Comments

@mcortez1497
Copy link
Contributor

Describe the Bug

Certain Helix styles have margin definitions that are dependent on spaces being present. This is mostly apparent when using React, since React strips out unneeded spaces.

Steps to Reproduce

Without React

(per Nicko Winner-Arroyo)

  1. Write Helix-UI markup without newlines:
    <hx-alert cta="burn it" status="spider">Nope! Nope! Nope! Nope! Nope!</hx-alert>
  2. This will produce an hx-alert element with proper spacing between SPIDER: Nope! ...

With React

  1. Write Helix-UI markup using React
  2. This will remove all newlines and "unnecessary" whitespace, resulting in Helix-UI elements mismatching the expected result in the documentation.

I've created a Codepen snippet that highlights a few of the inconsistencies I've found so far. They include:

  • Alert spacing between the status and text value.
  • Spacing in between buttons.
  • Spacing with hx-icons within buttons.

Expected behavior

The stylings should be the same whether or not whitespace is being used.

Screenshots

Whitespace nodes are easily apparent in Firefox. Each of the spacing issues seem to have a width of 4px.
screen shot 2018-07-11 at 3 06 38 pm

Whitespace nodes were not visible in the inspector when viewing the spacing for hx-alert's or hx-icons, though there is a 4px spacing that's visible.

Environment

Please complete the following information:

  • Device: Laptop
  • OS: OSX High Sierra 10.13.5
  • Browser: Chrome, Firefox
  • Version: Chrome Version 67, Firefox Developer Edition 54

Additional Context

Add any other context about the problem here.
@CITguy
Copy link
Contributor

CITguy commented Jul 13, 2018

JSX Behavior

The described behavior seems to be a result of how JSX compiles to HTML.

Element nodes will maintain white space when mixed with with non-element nodes on the same physical line of JSX code.

Sibling element nodes, on different physical lines of JXS code, are rendered without white space.

@mcortez1497 are you aware of any configuration options in React/JSX to modify this behavior?

Tricky Problem

The problem we have here is that we have to figure out how to support two different DOMs in CSS for the same palpable content.

natural (preserve significant whitespace, only remove newlines)

<p>
  Text Node
  <span>inline content</span>
</p>

compiled (all "unnecessary" whitespace removed)

<p>Text Node<span>inline content</span></p>

For the situation above, we have an inline element after text content. In order to consistently apply a margin between the text node and the <span>, we need the ability to target inline elements that immediately follow white-space-only text nodes. Unfortunately, no such capability exists in any browser.

In cases where the inline element appears before the text content, we'd need to be able to target an inline element that precedes a white-space-only text node. This is impossible to do in CSS, because selectors can only match forward (never backward).

@CITguy
Copy link
Contributor

CITguy commented Jul 13, 2018

I can think of two ways to potentially solve this.

  1. For React apps, add a .hxReact CSS class to <body> so that we can update CSS to style things appropriately. This will require auditing all CSS across HelixUI and there's no guarantee that we can find fixes for every scenario.

  2. Add the white-space-only nodes directly into helix-react components using { " " } in JSX. This is a much nicer solution, given that it'll be isolated to React consumption.

@CITguy CITguy mentioned this issue Jul 13, 2018
Closed
@CITguy
Copy link
Contributor

CITguy commented Jul 13, 2018

There may be instances where we can fix this with CSS in helix-ui, but I've created HelixDesignSystem/helix-react#5 to capture the need to strategically insert explicit white-space-only text nodes into JSX templates.

@CITguy
Copy link
Contributor

CITguy commented Jul 13, 2018 •
edited
Loading

Discoveries

  • Floating seems to remove an element from normal inline flow and ignores whitespace when applying margin.
  • Setting a parent container to inline-flex will ignore whitespace between nodes when margin is applied.

What can be fixed

  • Alert "status" can be corrected by floating the span to the left and applying a right margin.
  • Icons within buttons can be corrected by changing the display of the button to inline-flex and applying a left margin to the icon.

What cannot be fixed

Button spacing cannot be fixed with CSS alone, because we cannot guarantee that two sibling buttons are arranged side-by-side, horizontally. This will require adding explicit white-space-only text nodes ({" "}) in JSX markup.

@CITguy CITguy added the CSS label Jul 13, 2018
@mcortez1497
Copy link
Contributor Author

I've been looking at different loader options with Webpack, trying to see if there are any configuration options that allow for whitespace not to be trimmed in JSX, but there doesn't seem to be any.

Looking at some related issues (facebook/react#4134), it seems like explicitly adding text nodes would likely be the best course of action in helix-react.

@CITguy
Copy link
Contributor

CITguy commented Jul 14, 2018 •
edited
Loading

@mcortez1497: agreed. Though we'll still want to document compatibility quirks, just in case. I'm not sure if we can guarantee 100% coverage with helix-ui CSS changes and explicit text nodes in helix-react components, but I'm certain we can cover the vast majority of use cases.

Regardless, I'll be creating a defect story in JIRA to capture updating CSS in order to alleviate this issue as best we can via helix-ui.

(thinking out loud) I wonder if by always adding an explicit white-space-only text node to the end of every JSX template would be a good or bad idea. (probably depends on the component)

return (
  <button className="hxBtn">
  {this.props.children}
  </button>
  {' '}
)

@CITguy
Copy link
Contributor

CITguy commented Jul 16, 2018

Created SURF-1219 to capture work that can be completed in helix-ui. We can't guarantee that all cases can be corrected via helix-ui, so we'll be initializing a F.A.Q. to document compatibility with JSX.

@CITguy
Copy link
Contributor

CITguy commented Jul 16, 2018

Closing this story. We can continue discussions here, but necessary work should be documented in SURF-1219.

@CITguy CITguy closed this as completed Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CSS
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@CITguy @mcortez1497