Conversation
|
Hi @manishbisht, thank you so much for the PR! Can you attach some screenshots of the intended result for documentation purposes? |
diegoquinteiro
left a comment
diegoquinteiro
left a comment
There was a problem hiding this comment.
Thanks for your PR! Please consider the inline comments.
src/js/components/RuleLabel.react.js
Outdated
| const React = require('react'); | ||
| const level_1 = '•'; | ||
| const level_2 = '✘'; | ||
| const level_3 = '✔'; |
There was a problem hiding this comment.
Let's not use level numbers. Each symbol has a specific meaning for the field and we need to make the semantics clear.
• => Blank and not required
✘ => Required and blank
✔ => Field filled
Let's break level into 2 booleans (required and filled).
src/js/components/RuleLabel.react.js
Outdated
| @@ -0,0 +1,42 @@ | |||
| import type { Props } from '../containers/AppContainer.react'; | |||
| import { RulePropertyUtils } from '../utils/RulePropertyUtils'; | |||
There was a problem hiding this comment.
The Copyright note with the @flow annotation needs to be the first thing in the file, so please push the imports below it.
Doing like you did disables flow for this file.
src/js/components/RuleLabel.react.js
Outdated
| const level_2 = '✘'; | ||
| const level_3 = '✔'; | ||
|
|
||
| class RuleLabel extends React.Component<Props, State> { |
There was a problem hiding this comment.
Your class is stateless, so please use extends React.Component<Props>.
src/js/components/RuleLabel.react.js
Outdated
| <div> | ||
| {this.props.className && this.props.htmlFor | ||
| ? '<label className="' + | ||
| this.props.className + |
There was a problem hiding this comment.
In this case this.props is of type Props, which in this context is simply the Props type of the AppContainer. It does not have a className property.
You need to extend the container Props by importing it with a new name:
import type { Props as BaseProps } from '../containers/AppContainer.react';
type Props = BaseProps & { className: string ... Please check the example on PropertyPicker.rect.js.
There was a problem hiding this comment.
So I should add type Props = BaseProps & { className: string, htmlFor: string}; to use className and htmlFor property ?
src/js/components/RuleLabel.react.js
Outdated
| '" htmlFor="' + | ||
| this.props.htmlFor + | ||
| '">' | ||
| : '<label>'} |
There was a problem hiding this comment.
This will simply output the string '<label>' instead of the <label> element. Please remove the single quotes.
Also, and more importantly, please test your result with npm start before submitting a Pull Request, as this error is very apparent on the UI:
src/js/components/RuleLabel.react.js
Outdated
| : this.props.level === '2' ? level_2 : level_3} | ||
| </span>{' '} | ||
| {this.props.title} | ||
| {'</label>'} |
| <RuleLabel level="3" title="Selector" /> | ||
| ) : ( | ||
| <RuleLabel level="2" title="Selector" /> | ||
| )} |
There was a problem hiding this comment.
After migrating to 2 boolean properties, let's refactor all conditions like this to simply:
<RuleLabel filled={this.props.rule.selector != ''} required={true} title="Selector" />| : '•'} | ||
| </span> Audience Network ID | ||
| </label> | ||
| {this.props.settings.adsSettings.audienceNetworkPlacementId ? ( |
There was a problem hiding this comment.
For example, this conditional block becomes simply:
<RuleLabel
filled={this.props.settings.adsSettings.audienceNetworkPlacementId}
required={false}
title="Audience Network ID"
htmlFor="audienceNetworkId"
/>
Signed-off-by: manishbisht <manish.bisht490@gmail.com>
manishbisht
force-pushed
the
label-component
branch
from
f4a7155 to
e52a4a7
Compare
|
Thanks for the suggestions @diegoquinteiro !! I have made the changes. Can you review this PR now. |
|
@diegoquinteiro Any update on this PR ? |
3 participants
Fixes #88