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
yannickroger merged 1 commit into
ezsystems:masterfrom
miguelcleverti:exposeNavigationIdentifier
Aug 16, 2016
Merged

EZP-26120: Expose the navigation items identifier in the navigation hub#654
yannickroger merged 1 commit into
ezsystems:masterfrom
miguelcleverti:exposeNavigationIdentifier

Conversation

@miguelcleverti

@miguelcleverti miguelcleverti commented Jul 26, 2016

Copy link
Copy Markdown

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

dpobel commented Jul 26, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
render: function () {
var container = this.get('container');

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

Copy link
Copy Markdown
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
Copy Markdown
Author

@dpobel unit test added

);
},

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

Copy link
Copy Markdown
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

dpobel commented Jul 28, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

+1

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

Copy link
Copy Markdown
Author

ping @yannickroger @glye all green

@andrerom

Copy link
Copy Markdown
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