Skip to content
This repository has been archived by the owner on Jan 8, 2021. It is now read-only.

Pull request for issue #161 and #162 #199

Merged
merged 60 commits into from Sep 6, 2012
Merged

Pull request for issue #161 and #162 #199

merged 60 commits into from Sep 6, 2012

Conversation

aljana
Copy link
Contributor

@aljana aljana commented May 28, 2012

The most basic wall with input box and post listing. Not finished yet but so that you can see we are moving forward. Ideas welcome. :)

@mitar
Copy link
Member

mitar commented May 29, 2012

Please merge with main repository.

border: none;
outline: none;
resize: none;
font-family: Verdana, Arial, Helvetica, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

No spaces between commas?

Copy link
Member

Choose a reason for hiding this comment

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

TODO.

Copy link
Member

Choose a reason for hiding this comment

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

TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed that line, we don't need different font for the textarea...

@mitar
Copy link
Member

mitar commented May 29, 2012

OK, I don't know how to comment on this. Yes, it looks good. But it is a bit messy. Maybe first let us get all features in and then clean the code, no?

So we need post updates to all clients connected to the page. This is still missing. Otherwise it looks nice.

@mitar
Copy link
Member

mitar commented May 29, 2012

But you really have to update to latest version. It also uses newer version of our tastypie library with many bugfixes.

function generate_post_html(data) {
var li = $('<li/>').prop();

return '<li class="post"><span class="author">'+ data.author['username'] + '</span><p class="content">' + data.message + '</p><span class="date">'+ format_post_date(data.created_time) +'</span></li>'
Copy link
Member

Choose a reason for hiding this comment

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

Create HTML elements through jQuery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

@mitar
Copy link
Member

mitar commented Jun 1, 2012

Just to make sure, I am waiting for you here, no?

@mitar
Copy link
Member

mitar commented Sep 5, 2012

So the dates work for you now? Do you know what was wrong? Why you had problems with them?

@aljana
Copy link
Contributor Author

aljana commented Sep 5, 2012

The translations for dates and time diff calculations are now working fine. Translations started working after merging with upstream so I'm not sure what exactly was the problem.

@mitar
Copy link
Member

mitar commented Sep 5, 2012

In which way translations didn't work for you? So the only problem I noticed (after you did translations right) was that because of timezones, calculated delta was always negative between current time and post time, so "right now" was always displayed. (Probably you had some other time-span displayed, so some other time offset, because you are in the other timezone.) So when you write "does not work", you should explain how it does not work for you.

@aljana
Copy link
Contributor Author

aljana commented Sep 5, 2012

Yes I also noticed the problem with timezones and I fixed it. The other problem was that I wasn't getting Slovenian translations for "days, hours, minutes". Maybe it was cache. I don't know anymore.
Everything should be fine now. You can check. (I hope that's the last round).

@mitar
Copy link
Member

mitar commented Sep 5, 2012

It looks good now. I am still missing that you open tickets and pull requests. :-)

delete_link
);

var post = $('<li/>')
Copy link
Member

Choose a reason for hiding this comment

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

Better, but a bit too much now. :-0

var post = $('<li/>').addClass('post').data('post', self).append(
    post_options
).append(
    $('<span/>').addClass('author').text(self.author.username)
).append(
    $('<p/>').addClass('content').text(self.message)
).append(
    $('<span/>').addClass('date').text(formatPostDate(self.created_time))
);

@mitar
Copy link
Member

mitar commented Sep 5, 2012

So, do you remember which tickets we talked to open? Because I don't. :-)

@aljana
Copy link
Contributor Author

aljana commented Sep 5, 2012

Let me check.
As I understood the most important was the one with post creation multicast or something like that. So we don't send updates in the middle of request. It's described here #199 (comment). I think you should open this one, it's difficult to describe.
Other features are already written in TODOs like improving date updates and adding options (such as delete, edit) to post.

@mitar
Copy link
Member

mitar commented Sep 5, 2012

You can open tickets for important TODOs so that your colleagues can easier find it. Maybe just the one for dates.

@aljana
Copy link
Contributor Author

aljana commented Sep 5, 2012

Hmm I made a pull request for tastypie and now also all your and my comments appeared there? Didn't think of that. Should I delete them or is it okay to leave them?

@mitar
Copy link
Member

mitar commented Sep 5, 2012

I don't see them.

@mitar
Copy link
Member

mitar commented Sep 5, 2012

OK. I see them. He he. Leave. :-)

@mitar
Copy link
Member

mitar commented Sep 5, 2012

Where are other pull requests?

@mitar
Copy link
Member

mitar commented Sep 5, 2012

Please link them all here.

@mitar
Copy link
Member

mitar commented Sep 5, 2012

Good job!

mitar added a commit that referenced this pull request Sep 6, 2012
@mitar mitar merged commit ca65b44 into wlanslovenija:master Sep 6, 2012
@mitar
Copy link
Member

mitar commented Sep 6, 2012

See changes I did after merge.

@mitar
Copy link
Member

mitar commented Sep 18, 2012

Oh. Was this pull request a hidden mine. It is not good to just so upgrade dependencies to newest versions (and even unreleased ones). Things completely broke. Which would be quite obvious if would run the tests. Or, if you would run the tests. Even if we do not have many of them, they fail loudly.

@aljana
Copy link
Contributor Author

aljana commented Sep 18, 2012

Hmmm where exactly is the problem? Forgot about tests... :/

@mitar
Copy link
Member

mitar commented Sep 18, 2012

I fixed the problem by reverting to older versions. But simply, mongoengine in newer versions works a bit differently, so things fail. And tastypie changed API, so things fail fast and REST protocol do not work for subresources (like comments, attachments).

So the problem is when you change requirements, then you have to test if everything still works. This is why we have concrete versions hard-coded. For all modules.

@aljana
Copy link
Contributor Author

aljana commented Sep 18, 2012

Uhh that's not good. I'm sorry about that, I completely forgot about the tests, will know better next time. But this will have to be fixed once or we'll just stick to this versions forever?

@mitar
Copy link
Member

mitar commented Sep 19, 2012

I have also forgot about tests. This is why I have now integrated the repository with Travis CI. It will check automatically every time.

Yes, we will have to fix it. But this can be a ticket or another branch/pull request, not that we have broken code and everything does not work anymore in master, where others are working on it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants