[ckan-dev] Code review for 3009-on-site-notification-of-new-activity

Mon Nov 19 14:44:18 UTC 2012

On 19 November 2012 10:41, Sean Hammond <sean.hammond at okfn.org> wrote:

> > > Hey, could we get someone to review this soon?
> > >
> > > https://github.com/okfn/ckan/pull/168
> > >
> > > It's ready for review now and it's blocking two other dev branches.
> > >
> >
> > I'll try to finish reviewing/merge it on monday
>
>
Sean,

I've had a look at this branch and although I'm not 100% happy I think we
should merge it as is so we get some user testing/feedback

The main issue I have is regarding the 'user area' in the header it is pig
ugly in my opinion this may be intentional (yes i ran bin/less), also the
html is incorrect as the notification  div has a class="dropdown" which it
certainly isn't.

Those fixes can easily be done outside this branch.

the follow/unfollow actions do not have auth functions though this may not
be entirely needed and there are some direct calls to auth functions as
mentioned in the pull request.  These can probably wait till the 2939
branch is merged

Also it would be good to check the functionality in the legacy templates a
little more as I have mainly looked from the new template side though I did
do some checking with the old ones

Just running the final tests

Toby

Thanks Toby!
>
> _______________________________________________
> ckan-dev mailing list
> ckan-dev at lists.okfn.org
> http://lists.okfn.org/mailman/listinfo/ckan-dev
> Unsubscribe: http://lists.okfn.org/mailman/options/ckan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.okfn.org/pipermail/ckan-dev/attachments/20121119/66ccf0fc/attachment-0001.html>