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

fix: time series table #7302

Merged
merged 4 commits into from
Apr 16, 2019
Merged

fix: time series table #7302

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
955a4d1
fix: time series table
khtruong Apr 15, 2019
4cbfc9b
fix: add default value for label
khtruong Apr 15, 2019
67136cd
fix: use prop values if defined
khtruong Apr 15, 2019
071b242
fix: revert CollectionControl changes
khtruong Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion superset/assets/src/explore/components/controls/TimeSeriesColumnControl.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import BoundsControl from './BoundsControl';
import CheckboxControl from './CheckboxControl';

const propTypes = {
label: PropTypes.string,
onChange: PropTypes.func,
};

Expand All @@ -52,7 +53,21 @@ const colTypeOptions = [
export default class TimeSeriesColumnControl extends React.Component {
constructor(props) {
super(props);
const state = { ...props };
const state = {
label: this.props.label,
tooltip: '',
colType: '',
width: '',
height: '',
timeLag: '',
timeRatio: '',
comparisonType: '',
showYAxis: false,
yAxisBounds: [null, null],
bounds: [null, null],
d3format: '',
dateFormat: '',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously state was set to what's in props const state = { ...props }, should these be set to the values in props instead of nulls and empty strings? It would also be good to explicitly state what's in props and list them as either required or list them in defaultProps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None of the properties in props correspond to state properties except label.

Screen Shot 2019-04-15 at 12 56 55 PM

The line I removed is definitely confusing to read. It leads the reader to believe that props has values like "colType", "width", etc. when in actuality it has none of these properties.

Listing out the state properties should improve readability.

I've also added a default value for label in props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can also see in the screenshot all the random properties we add to state that leads to a bad request to the backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding null values and empty strings, can we just omit them? The consumer of that object should be the thing that decides a) if it wants those values to be there and b) what to do if they are missing. Inserting them here seems like it is coupling things in a way that is going to be fragile

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, @khtruong I think that works just as a declaration of "here are the fields and initial values that this type cares about". I was thinking it was being used differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you printing out the this.props from CollectionControl? When I look at props from within the constructor of TimeSeriesColumnControl when loading a time series table chart I see the other props like colType d3format set that correspond to state properties. You mentioned in chat that o passed to the Control is undefined, on master it looks like it's getting set correctly. Are there any changes in lyftga that could have affected this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see what is going on. I was looking into this.props and you were referring to this.props.value. Yes, I can set the state to what is stored in this.props.values.

Also ignore my comment about o being undefined. I was within the map function's scope when I tried to look up this.props which was undefined within that scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked to Michelle offline. Turns out she was looking at the props with Max's changes and I was looking at the props without Max's changes. Hehe.

Now I have set state to the prop values if they exist. Otherwise, they are sent to default values.

delete state.onChange;
this.state = state;
this.onChange = this.onChange.bind(this);
Expand Down