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

Backend and frontend changes #2

Merged
merged 52 commits into from
Jun 21, 2016
Merged

Backend and frontend changes #2

merged 52 commits into from
Jun 21, 2016

Conversation

ghoshbishakh
Copy link
Collaborator

No description provided.

@ghoshbishakh
Copy link
Collaborator Author

more changes coming

}


// carousal-main styles
Copy link
Collaborator

Choose a reason for hiding this comment

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

carousal -> carousel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing. fixed

@jchoude
Copy link
Collaborator

jchoude commented Jun 3, 2016

Apart from my small comments, seems good. I'll wait on the fixes / replies to comments and on @Garyfallidis to merge this.

GJ 👍

@ghoshbishakh ghoshbishakh changed the title Disallow null values for website_position_id in WebsiteSection Backend changes, mostly for admin panel and cms Jun 6, 2016
url = models.CharField(max_length=200)
author = models.CharField(max_length=200)
doi = models.CharField(max_length=100, null=True, blank=True)
journal = models.CharField(max_length=200, null=True, blank=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you intend on keeping track of the type of publication (conference abstract, journal paper, etc)?

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 have looked at bibtex of several publications but could not find any field that groups them as conference and journal paper. So I think we will have to enter it manually. I can provide a field for that.

Copy link
Collaborator Author

@ghoshbishakh ghoshbishakh Jun 16, 2016

Choose a reason for hiding this comment

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

I have added a field to keep track of publication type like book, article, inproceedings etc. in commit 60e95d4

@jchoude
Copy link
Collaborator

jchoude commented Jun 15, 2016

General question, which is related to the discussion we had earlier today: do we really need people to be able to modify the installation, overview, etc sections directly in the admin page using the editor? I'm not against it, I'm just wondering if it's really more complicated to just update the content in the page in the git repo, and just push that.

Again, maybe it's to make it easier for everyone to maintain. I don't remember if it was already discussed.

@@ -0,0 +1,120 @@
from django.shortcuts import render, redirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't remember if we agreed on using PEP8 for .py files, but normally imports should be ordered alphabetically.

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 am trying to maintain PEP8 . So should the imports be like
from django.shortcuts import redirect, render

or is it with the other imports like

from website.forms import AddEditCarouselImageForm

from website.models import CarouselImage

from .tools import has_commit_permission

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is mainly to order each line, not necessarily the elements of each line. So, more like your second case.

Also, officially (https://www.python.org/dev/peps/pep-0008/#imports), if you look at the third point, they should be separated in 3 blocks: system / basic python imports first, then the third-party imports (like Django), and then the imports from the current project.

@jchoude
Copy link
Collaborator

jchoude commented Jun 15, 2016

Really good progress! Good job.

Also, quick note, I went quite fast through the CSS, because this is not something I can easily "view" just in my head, like this. The website has a nice layout and renders well on my setup, so that's a good first step.

@ghoshbishakh ghoshbishakh changed the title Backend changes, mostly for admin panel and cms Backend and frontend changes Jun 18, 2016
@jchoude
Copy link
Collaborator

jchoude commented Jun 21, 2016

Ok. @ghoshbishakh I don't know if you have any other commits lined-up for this PR, but it is getting quite huge. I'd like to merge it, else it will not be reviewable. So, please tell me if you absolutely need to add other commits, or if I can make another quick review of the new commits, and then merge.

Also, for your next work, please make sure you create branches, instead of working in your master branch. That will make the workflow easier. You will also be able to make quick, small branches, leading to small PRs.

@ghoshbishakh
Copy link
Collaborator Author

You may merge it. I will make a separate PR for the rest of the changes
from a new branch.

On Tue, Jun 21, 2016 at 6:45 PM, Jean-Christophe Houde <
[email protected]> wrote:

Ok. @ghoshbishakh https://github.com/ghoshbishakh I don't know if you
have any other commits lined-up for this PR, but it is getting quite huge.
I'd like to merge it, else it will not be reviewable. So, please tell me if
you absolutely need to add other commits, or if I can make another quick
review of the new commits, and then merge.

Also, for your next work, please make sure you create branches, instead of
working in your master branch. That will make the workflow easier. You will
also be able to make quick, small branches, leading to small PRs.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AE6vs81JVD9R_poYHjKIlYI4BeVGt-KEks5qN-QGgaJpZM4ImzY8
.

@jchoude
Copy link
Collaborator

jchoude commented Jun 21, 2016

Cool, thanks. I went through the last commits after the previous review I made. A batch of questions or issues. For issues, you can tag them in the issue tracker and tackle them after the merge. I will merge, but you still can (and should) answer my questions here.

  • When I edit a page / section, the lines, words count at the bottom of the editor always shows 0 until I start editing. Then, they show the correct values. It's a very minor issue.
  • In dashboard/sections/page/, it would be nice to have less content, i.e., not show the whole pages' content. Either a small part of the beginning of the page, or just the title of the page. That way, we don't need to scroll endlessly to get to the edit button of the installation page, for example.
  • Remember to set debug=False in your settings before we release the page in the wild. It is set to true on Heroku...
  • On the current version on Heroku, there is nothing in the G+ feed. Expected?
  • I saw that, in one commit, you added a separate honeycomb page. There is also the support page, with gitter support. There only is the support page that I can reach, and only by clicking on the icon in the front page. I would personnally put it in the navbar.
  • The "Install now" icon in the index currently points to a dead link for the installation page.
  • For my personal knowledge, what is the difference when using the .scss file instead of the .css one?

@jchoude jchoude merged commit e5e8d04 into dipy:master Jun 21, 2016
@ghoshbishakh
Copy link
Collaborator Author

  1. I will look into it why it shows 0 atfirst.
  2. I will truncate the posts after a certain length then.
  3. Yes in production there is a pretty long checklist to follow. That includes turning debug off.
  4. Google plus api requires to put trusted IP addresses. I am still unable to find one for heroku. It is working locally for me. I hope heroku gives a static ip.
  5. Currently there are too many links in the navbar. I am thinking of using drop down menus instead of single links.
  6. Missed that. WIll change that in next commit.
  7. Scss has some extra features like nesting, mixing, variables etc. The one I use most is nesting. I plan to include vendor specific prefix mixing (-webkit, -moz, etc.) later.

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.

2 participants