Skip to content

Conversation

@avalanchy
Copy link
Contributor

@avalanchy avalanchy commented Oct 25, 2017

By looking at the template source code, I find out that we have forgotten option to create a horizontal rule between options. Here I repaired it and documented it. I also allowed myself to rename it to "divider" as this name seems to better explain the role. On the other hand, I can at this point break the backward compatibility. What do you think?

selection_113

@andybak
Copy link
Member

andybak commented Oct 25, 2017

It wasn't actually broken before - just undocumented and maybe unintuitive!

The idea is that you add the property 'divided' to any menu item and it creates a horizontal rule before itself. Hence the use of the word 'divided' and the way the if/else was constructed in the template.

I actually prefer your solution but I'm a bit wary of breaking backwards compatibility.

I guess our options are:

  1. Support both for perpetuity.
  2. Switch to your syntax after a deprecation period with a warning.
  3. Stick to the current method and document it.

What do you think?

@avalanchy
Copy link
Contributor Author

Yup, it is a bit unintuitive - that's why I created the "divider" :) But with its usage documented - no one should have troubles with using it.

I tested the existing implementation and yeah, it works fine!

I allowed myself to force push the branch with the different commit that only updates documentation. I don't really wanna add support for both options since they will not give extra value to this plugin and most probably instead of bringing simpler API, will complicate the source code.

@avalanchy avalanchy changed the title Repair and expose undocumented horizontal rule option Document 'divided' option Oct 26, 2017
@andybak andybak merged commit 2778fc4 into DjangoAdminHackers:master Oct 26, 2017
@andybak
Copy link
Member

andybak commented Oct 26, 2017

I'm happy with that.

@avalanchy
Copy link
Contributor Author

Cool. Thanks.

@avalanchy avalanchy deleted the divider branch October 26, 2017 17:02
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