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

display date of comments [FIX #1021] #1283

Merged
merged 2 commits into from Dec 7, 2017
Merged

display date of comments [FIX #1021] #1283

merged 2 commits into from Dec 7, 2017

Conversation

taniki
Copy link
Contributor

@taniki taniki commented Nov 29, 2017

No description provided.

@@ -148,7 +151,11 @@ export default {
} else {
this.$scrollTo(this);
}
},
formatDate(val) {
return moment(val).format('LL')
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately as far as I'm concerned, we're not using StandardJS conventions, so I think we need a ; here.

@abulte
Copy link
Contributor

abulte commented Nov 29, 2017

Screenshots? 🙇

@taniki
Copy link
Contributor Author

taniki commented Nov 29, 2017

@abulte

image

@taniki
Copy link
Contributor Author

taniki commented Nov 29, 2017

I have added some missing semicolons in the other part of the template. After some thought, this certainly could be a global filter.

Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

If you made sure it works on mobile too, 👌 for me.

@taniki
Copy link
Contributor Author

taniki commented Nov 30, 2017

image

@abulte there is a legacy .ellipsis class on that part of the content. I am thinking about removing it before I might miss a point about why it what there first.

@abulte
Copy link
Contributor

abulte commented Nov 30, 2017

It's not too bad as is.

Copy link
Contributor

@pblayo pblayo left a comment

Choose a reason for hiding this comment

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

A question.

@@ -97,10 +100,10 @@ export default {
return `discussion-${this.discussion.id}`;
},
createdDate() {
return moment(this.discussion.created).format('LL')
return moment(this.discussion.created).format('LL');
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it intended to modify this line to add a semicolon while the function formatDate is added below with a very similar code:

return moment(val).format('LL');

Is it to avoid an indirection that formatDate isn't used here ?

Copy link
Contributor

@pblayo pblayo Dec 3, 2017

Choose a reason for hiding this comment

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

(it's minor, I marked it as "change requested" to make it visible because I'm not sure of the etiquette of this repository yet, maybe a "comment" would have been more appropriate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep it was intended but only to comply with @abulte comment about not using StandardJS (which promote not using semi).

The best workaround for this case would be to implement a global filter but I am still a bit shy about code impact.

@taniki taniki self-assigned this Dec 4, 2017
@taniki taniki merged commit 8006ef8 into opendatateam:master Dec 7, 2017
@taniki
Copy link
Contributor Author

taniki commented Dec 7, 2017

Sorry I forgot to use the in-github squash option.

@noirbizarre noirbizarre added this to the 1.2.5 milestone Dec 7, 2017
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.

None yet

4 participants