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

[Behat] Improve CommonActions and Authentication context Css selectors #653

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

miguelcleverti
Copy link

In order to stop fetching the elements using their text value the css selectors needed some changes.

This PR adds some specific tag to the PlatformUI markup in order to better fetch the elements. Also some unused methods were cleaned up, since their were not being used anymore.

@miguelcleverti
Copy link
Author

@miguelcleverti miguelcleverti force-pushed the CommonActionSelectors branch from 7a2a9fa to 6b952e4 Compare July 25, 2016 14:53
@andrerom
Copy link
Contributor

looks like a good direction, as for the details here @bdunogier would have more insight on bdd part and @dpobel / @yannickroger on templates parts.

@@ -1 +1 @@
<a href="{{ path route.name route.params }}" class="ez-navigation-item">{{ title }}</a>
<a href="{{ path route.name route.params }}" class="ez-navigation-item" data-navigation="{{ title }}">{{ title }}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the point of that ? ok you are not searching by text content, but you are still searching by text which can be translated or changed for whatever reason. Actually, each navigation item has a unique identifier so we should probably expose it somewhere so that you can use it in the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@dpobel okay, I have already done this, should I include it in this PR or in a separate one?

Copy link
Author

Choose a reason for hiding this comment

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

I made it in a separate PR so it's easier to review #654

@miguelcleverti
Copy link
Author

ping @dpobel, @bdunogier. After trying some different way I found that in order to use the node-id in content tree elements would need a lot of code scattered by multiple contexts. I made a some tests and moving the title attribute to the parent element does not seem to break anything and this simplify the code greatly. What do you think?

@bdunogier
Copy link
Member

Could you link to the change you're referring to @miguelcleverti ?

@@ -1,7 +1,7 @@
<ul class="ez-tree-level">
{{#each children}}
<li class="ez-tree-node{{#if state.leaf}} is-tree-node-leaf{{/if}} {{#if state.open}}is-tree-node-open{{else}}is-tree-node-close{{/if}}" data-node-id="{{id}}">
Copy link
Author

Choose a reason for hiding this comment

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

@bdunogier, @dpobel these changes

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of moving the title from the link to the list item ? I mean you can find the link and just retrieve the parent node to get the list item. I'm saying that because with this change, you'll end up with something like:

<li title="A nice Content in the tree">
    <a href="#" <!-- ... -->>A nice Content in the tree</li>
    <ul class="ez-tree-level">
        <li title="A subitm of a nice Content in the tree">
        <!-- ... -->
        </li>
    </ul>
</li>

then the title is also associated with the subitems (the ul) of a given Location which is a bit weird semantically speaking. Also the li represents a Location in the tree while the a represents the Content item so the name (title) should stay at the Content item level since a Location has no name.

@bdunogier
Copy link
Member

Looks okay to me. @dpobel ?

@miguelcleverti
Copy link
Author

ping @dpobel @bdunogier @joaoinacio. Removed the previous changes to the title attribute in the content tree elements. Now the selecting of the elements is done using the data-node-id and the node id is fetched using the search service.

@andrerom
Copy link
Contributor

Seems you might want to rebase the PR or something, as there are some behat failures here.

@miguelcleverti miguelcleverti force-pushed the CommonActionSelectors branch 2 times, most recently from c1b1da1 to 62ff951 Compare October 18, 2016 09:45
@miguelcleverti miguelcleverti force-pushed the CommonActionSelectors branch 3 times, most recently from 243d106 to 2ce2c82 Compare October 26, 2016 13:07
@miguelcleverti
Copy link
Author

All the Behat test failures are due to Deprecation warning. It is ready to be reviewed.

@andrerom
Copy link
Contributor

andrerom commented Oct 27, 2016

Master is not failing on behat, could you rebase on top of latests? and if you introduce deprecation issues please fix them ;)

Besides that +1 so far :)

@miguelcleverti
Copy link
Author

PR approved by QA

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