Skip to content
This repository was archived by the owner on Oct 28, 2020. It is now read-only.

[fix bug 980140] Add goals field in events.#671

Merged
akatsoulas merged 1 commit into
mozilla:masterfrom
akatsoulas:980140
Mar 20, 2014
Merged

[fix bug 980140] Add goals field in events.#671
akatsoulas merged 1 commit into
mozilla:masterfrom
akatsoulas:980140

Conversation

@akatsoulas
Copy link
Copy Markdown
Contributor

No description provided.

@johngian
Copy link
Copy Markdown
Contributor

I have taken a look and this PR seems to work fine. I have some thoughts about the icon in the edit event page. @alexgibson Any thoughts?

Comment thread remo/events/models.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please correct typo.

@alexgibson
Copy link
Copy Markdown
Contributor

A couple of minor nits, but front-end looks fine to me r+wc. 🚀

As discussed on IRC, we are going to opt for the 'medal' icon for the goals button.

@johngian
Copy link
Copy Markdown
Contributor

Just a heads up:

We need to have event goals in the DB by the time we launch this feature, else users cannot edit events since this field is required.

@johngian
Copy link
Copy Markdown
Contributor

Also we need to make sure that we have event goals available for old events as well, in case someone edits an old event.

Comment thread remo/events/admin.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be useful to register the EventGoal model in admin except of that inline in events.

@johngian
Copy link
Copy Markdown
Contributor

r+wc 🐰

akatsoulas added a commit that referenced this pull request Mar 20, 2014
[fix bug 980140] Add goals field in events.
@akatsoulas akatsoulas merged commit c3d6b03 into mozilla:master Mar 20, 2014
@akatsoulas akatsoulas deleted the 980140 branch April 16, 2014 10:09
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