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

Added a "target" option to specify the target attribute for the menu links #907

Merged
merged 5 commits into from
Feb 20, 2016

Conversation

javiereguiluz
Copy link
Collaborator

This pull request finishes the work done originally by @maldoinc in #845.

maldoinc and others added 4 commits January 29, 2016 11:05
Useful when wanting to open a link in a new tab where you simply set the `target` key to `_blank`
Added option to add target attribute to menu links
@Pierstoval
Copy link
Contributor

I don't like this feature. It's "specific" behavior for something a bit restrictive.

Why not add a attr option that will contain all HTML properties to the tag? (of course with the href one being overriden to avoid issues)

@ogizanagi
Copy link
Contributor

I agree with @Pierstoval on this.

@javiereguiluz
Copy link
Collaborator Author

I understand the concerns of @Pierstoval and @ogizanagi ... but I still think is OK to add this small feature. I don't plan to add more configuration options for menu items, so this is the last one (and that means that we won't clutter the menu feature). Besides, we allow links to absolute URLs and external systems in the menu, so it's important to be able to open them in new tabs/windows.

The problem with attr is that it forces you to remember which options are first-level options and which ones nested inside attr. And nested arrays are ugly in YAML files. We do that when we have no other option (line type_options for form fields) but we try to avoid that as much as possible.

So, I'm going to merge this feature. But, in the future, we can reconsider this decision. And since the feature is minimal, and removing it won't break the backend, BC breaks won't matter. Thanks!

@javiereguiluz javiereguiluz merged commit 5a00dae into master Feb 20, 2016
javiereguiluz added a commit that referenced this pull request Feb 20, 2016
…for the menu links (maldoinc, javiereguiluz)

This PR was merged into the master branch.

Discussion
----------

Added a "target" option to specify the target attribute for the menu links

This pull request finishes the work done originally by @maldoinc in #845.

Commits
-------

5a00dae Added tests for the feature
13c1e58 Fixed tests
84584b3 Tweaked the new feature
af018e0 Merge pull request #1 from maldoinc/maldoinc-patch-menu-target
142531f Added option to add target attribute to menu links
@Pierstoval
Copy link
Contributor

The problem with attr is that it forces you to remember which options are first-level options and which ones nested inside attr. And nested arrays are ugly in YAML files. We do that when we have no other option (line type_options for form fields) but we try to avoid that as much as possible.

Precisely, having attr is a good standard because it allows everyone to know what's the difference between logical parameters and html parameters. attr is also used in Symfony forms with attr for widgets and label_attr for labels for example.

Here, when you add "magic" (= a seemingly "logical" option that's transformed into a basic html parameter) you induce bc breaks when the attr option is finally introduced.

I personally have to change the class of one link in some of my backends, and another will use javascript so it needs an id and an empty href attribute.

There's always room for full flexibility, especially when you follow good/standard practices that are used elsewhere the same way.

And for the And nested arrays are ugly in YAML files , there is not much solution for this. Actually, there is one: define the configuration from tagged services returning arrays (something I proposed last year many times).

@javiereguiluz javiereguiluz deleted the pr/845 branch February 26, 2016 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants