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

New menu config #52

Closed
jonashaag opened this issue Mar 22, 2013 · 4 comments
Closed

New menu config #52

jonashaag opened this issue Mar 22, 2013 · 4 comments

Comments

@jonashaag
Copy link
Contributor

First feedback: AWESOME! This is really great stuff. Works very well! A few minor things:

  1. When using 'url': 'app.model', the active menu item is only highlighted on the model list page, not on instance edit form pages.
  2. I don't think '/' is a good indicator for a separator. Why not use something explicit, maybe 'SPACER' or '---'?
  3. 'url': 'app.model' seems a bit like magic to me: If the model in question is not found, you'll just the get URL /app.model/ as link target. I suggest making things more explicit here too, maybe using 'model': 'myapp.mymodel'.
  4. Could you add the possibility to open a menu item in a new window? I understand that this is a very specific use case but maybe you think it's useful.
@darklow
Copy link
Owner

darklow commented Mar 22, 2013

Thanks for testing.
I'll fix 1.
2. I choose '/' because that is separator for many wysiwyg editors like ckeditor. I vote for '-' then :)
3. If somebody uses 'url': 'app.model' i don't think it can be by accident, so it means model must be there. I don't like idea of mixing model key also in app level, it is too confusing. This parameter sets nothing but url so i think url key is the right for it.
4. It is quite specific, but i guess it could be useful for others too, for example when linking separate applications/admins together. What do you think, should i add blank=True or more universal target=_blank, i doubt if somebody uses iframes and could need different target than _blank...

@jonashaag
Copy link
Contributor Author

3.: The thing is that you end up having wrong URLs if you have a typo (--> model doesn't exist). On the other hands, it's pretty easy to check for typos in links by just testing them :)

4.: Yeah don't do the target=... thing. I don't think we have to support anything that has anything to do with frames ;)

darklow added a commit that referenced this issue Mar 23, 2013
Also improved tests for model activation
@darklow
Copy link
Owner

darklow commented Mar 23, 2013

Could you please test latest version?

@jonashaag
Copy link
Contributor Author

Awesome! Works perfectly.

phihos pushed a commit to phihos/django-suit that referenced this issue Mar 26, 2013
phihos pushed a commit to phihos/django-suit that referenced this issue Mar 26, 2013
Also improved tests for model activation
phihos pushed a commit to phihos/django-suit that referenced this issue Mar 26, 2013
phihos pushed a commit to phihos/django-suit that referenced this issue Mar 28, 2013
phihos pushed a commit to phihos/django-suit that referenced this issue Mar 28, 2013
Also improved tests for model activation
phihos pushed a commit to phihos/django-suit that referenced this issue Mar 28, 2013
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

No branches or pull requests

2 participants