Skip to content

feat(FelaComponent): Allow style prop with empty value#675

Merged
robinweser merged 1 commit intomasterfrom
feature/allow-empty-style-prop-in-felaComponent
Feb 20, 2019
Merged

feat(FelaComponent): Allow style prop with empty value#675
robinweser merged 1 commit intomasterfrom
feature/allow-empty-style-prop-in-felaComponent

Conversation

@TxHawks
Copy link
Copy Markdown
Collaborator

@TxHawks TxHawks commented Feb 19, 2019

Description

Since v10, FelaComponent throws if the style prop is null or undefined, which I think is wrong.

It's quite an edge case, but sometimes one may want an element to only be styled under certain conditions, and have that be determined dynamically.

The easiest and most straightforward way to do that is to pass null to FelaComponent's style prop from the parent component.

By removing the check and throw statement, it works as expected, so I don't really see any benefit in having it there. It doesn't really add any DX value, and is overly restrictive

Example

function MaybeStyled({ style, children, }) {
  return (<FelaComponent style={style}>{children}</FelaComponent>);
}


<MaybeStyle style={null} /> // renders <div></div>
<MaybeStyle style={{ color: 'red', }} /> // renders <div class="a"></div>

Packages

fela-bindings

Versioning

Patch

Checklist

Quality Assurance

You can also run yarn run check to run all 4 commands at once.

  • The code was formatted using Prettier (yarn run format)
  • The code has no linting errors (yarn run lint)
  • All tests are passing (yarn run test)
  • There are no flow-type errors (yarn run flow)

Changes

If one of the following checks doesn't make sense due to the type of PR, just check them.

  • Tests have been added/updated
  • Documentation has been added/updated
  • My changes have proper flow-types

@TxHawks TxHawks requested a review from robinweser February 19, 2019 19:26
@robinweser
Copy link
Copy Markdown
Owner

The point was that you don't need a wrapping FelaComponent without a style as you can simply render a div as well. But fair enough, it seems like a legit use case to pass undefined / null.

Copy link
Copy Markdown
Owner

@robinweser robinweser left a comment

Choose a reason for hiding this comment

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

I will merge it in and release for now, but maybe we should log a warning in devMode? So people see that path and might fix it later on. I don't think sth. like that will ever get a perf issue, but after all we render unnecessary components here.

@robinweser robinweser merged commit c27854b into master Feb 20, 2019
@robinweser robinweser deleted the feature/allow-empty-style-prop-in-felaComponent branch February 20, 2019 09:32
@TxHawks
Copy link
Copy Markdown
Collaborator Author

TxHawks commented Feb 20, 2019

but maybe we should log a warning in devMode?

Happy to do this. Did you release already?

@robinweser
Copy link
Copy Markdown
Owner

Not yet, but was about to do that soonish. But I'll have another release at the end of the week anyways :P

@TxHawks
Copy link
Copy Markdown
Collaborator Author

TxHawks commented Feb 20, 2019

Actually, how would you know if we are in dev mode inside FelaComponent?

@robinweser
Copy link
Copy Markdown
Owner

renderer.devMode

@TxHawks
Copy link
Copy Markdown
Collaborator Author

TxHawks commented Feb 20, 2019

Work just got really crazy, I don't think I'll manage to get to it today. Let's jus make sure to do this later

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