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.

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

Migrate "time ago" components to React #3385

Merged
merged 8 commits into from
Feb 2, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
91 changes: 91 additions & 0 deletions client/app/components/TimeAgo.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import moment from 'moment';
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';

const interactiveComponents = new Set();

function updateInteractiveComponents() {
interactiveComponents.forEach(component => component.forceUpdate());
setTimeout(updateInteractiveComponents, 3000);
arikfr marked this conversation as resolved.
Show resolved Hide resolved
}
updateInteractiveComponents();
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved

export class TimeAgo extends React.Component {
static propTypes = {
date: PropTypes.oneOfType([
PropTypes.string,
PropTypes.number,
PropTypes.instanceOf(Date),
// `moment`
(props, propName, componentName) => {
const value = props[propName];
if ((value !== null) && !moment.isMoment(value)) {
return new Error('Prop `' + propName + '` supplied to `' + componentName + '` should be a Moment.js instance.');
}
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved
},
]),
Copy link
Member

Choose a reason for hiding this comment

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

Why not enforce Moment only and let the caller do the conversion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes component more universal - so why not? We anyways convert this value to moment (yes, we can skip this, but it will not reduce LoC count).

Copy link
Member

Choose a reason for hiding this comment

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

One reason is that it makes it more confusing to use -- when looking at the prop definition, it's not clear what kind of string/number I can pass in. But if I see it accepts a Moment object, I'll figure out how to convert my value to moment.

But 🤷‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add a comment; you can pass everything accepted by moment

interactive: PropTypes.bool,
placeholder: PropTypes.string,
};

static defaultProps = {
date: null,
interactive: true,
placeholder: '',
};

constructor(props) {
super(props);
this._value = moment(this.props.date);
}

componentDidMount() {
if (this.props.interactive) {
interactiveComponents.add(this);
}
}

shouldComponentUpdate(nextProps) {
this._value = moment(nextProps.date);
if (nextProps.interactive) {
interactiveComponents.add(this);
} else {
interactiveComponents.delete(this);
}
return true; // default behaviour
}
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved

componentWillUnmount() {
interactiveComponents.delete(this);
}

render() {
return this._value.isValid() ? this._value.fromNow() : this.props.placeholder;
}
}

export default function init(ngModule) {
ngModule.directive('amTimeAgo', () => ({
link($scope, element, attr) {
const modelName = attr.amTimeAgo;
$scope.$watch(modelName, (value) => {
ReactDOM.render(<TimeAgo date={value} />, element[0]);
});
},
}));

ngModule.component('rdTimeAgo', {
bindings: {
value: '=',
},
controller($scope, $element) {
$scope.$watch('$ctrl.value', () => {
// Initial render will occur here as well
ReactDOM.render(<TimeAgo date={this.value} placeholder="-" />, $element[0]);
});
},
});
}

init.init = true;
17 changes: 0 additions & 17 deletions client/app/components/rd-time-ago.js

This file was deleted.

2 changes: 0 additions & 2 deletions client/app/config/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import ngMessages from 'angular-messages';
import toastr from 'angular-toastr';
import ngUpload from 'angular-base64-upload';
import vsRepeat from 'angular-vs-repeat';
import 'angular-moment';
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved
import 'brace';
import 'angular-ui-ace';
import 'angular-resizable';
Expand Down Expand Up @@ -49,7 +48,6 @@ const requirements = [
uiBootstrap,
ngMessages,
uiSelect,
'angularMoment',
toastr,
'ui.ace',
ngUpload,
Expand Down
43 changes: 26 additions & 17 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
"angular": "~1.5.8",
"angular-base64-upload": "^0.1.23",
"angular-messages": "~1.5.8",
"angular-moment": "^1.1.0",
"angular-resizable": "^1.2.0",
"angular-resource": "~1.5.8",
"angular-route": "~1.5.8",
Expand Down