Skip to content
This repository was archived by the owner on May 3, 2022. It is now read-only.

EZP-26120: Expose the navigation items identifier in the navigation hub #654

Merged

Conversation

miguelcleverti
Copy link

@miguelcleverti miguelcleverti commented Jul 26, 2016

As discussed on PR #653 the navigation items identifier should be exposed in order to the improve element searching in behat tests.

jira: https://jira.ez.no/browse/EZP-26120

@dpobel
Copy link
Contributor

dpobel commented Jul 26, 2016

Can you please include the issue number in the commit and in the PR title and in the pull request with a link to the issue (and move the issue on the board) ?

Also this is not the correct approach, as mentioned in the issue description, we should add the identifier in a data attribute on the view container. This would avoid adding such logic in the template and would make this behavior more robust when extending this class (it is meant for that).

(In addition, your patch is probably breaking a unit tests and you'll have to add some unit tests as well).

@miguelcleverti miguelcleverti force-pushed the exposeNavigationIdentifier branch from 5a6fd39 to 3c527c8 Compare July 26, 2016 10:43
@miguelcleverti
Copy link
Author

ping @dpobel if I understand correctly this should do it, if not could you provide an example.

@miguelcleverti miguelcleverti changed the title [Behat] Expose navigation items identifier in the markup EZP-26120: Expose the navigation items identifier in the navigation hub Jul 26, 2016
@@ -51,6 +51,7 @@ YUI.add('ez-navigationitemview', function (Y) {
render: function () {
var container = this.get('container');

container.setAttribute('data-navigation-item-identifier', this.get('identifier'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move that to the initializer where the containerTemplate is defined. Also this needs to be unit tested.

@miguelcleverti miguelcleverti force-pushed the exposeNavigationIdentifier branch from 3c527c8 to cdcfa93 Compare July 28, 2016 09:30
@miguelcleverti
Copy link
Author

@dpobel unit test added

@@ -44,6 +46,19 @@ YUI.add('ez-navigationitemview-tests', function (Y) {
);
},

"The Container should have a data identifier": function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Should add the navigation identifier on the container"

@dpobel
Copy link
Contributor

dpobel commented Jul 28, 2016

besides the 2 nitpicks +1
ping @StephaneDiot @yannickroger

@miguelcleverti miguelcleverti force-pushed the exposeNavigationIdentifier branch 2 times, most recently from 20f0f46 to 304cced Compare July 28, 2016 10:19
@StephaneDiot
Copy link
Contributor

+1

@miguelcleverti miguelcleverti force-pushed the exposeNavigationIdentifier branch from 304cced to 384f7b2 Compare August 16, 2016 13:34
@miguelcleverti
Copy link
Author

ping @yannickroger @glye all green

@andrerom
Copy link
Contributor

+1

@yannickroger yannickroger merged commit f57c978 into ezsystems:master Aug 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants