Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

step-15 Accessibility #341

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

step-15 Accessibility #341

wants to merge 16 commits into from

Conversation

felixzapata
Copy link

  • Add ngAria module.
  • Add labels to the search and order fields.
  • Add accessibility plugin for Protractor.
    • Add missing alt attributes in the phone detail.
  • Add aria live regions to inform the user about results after searching and filtering elements.
  • Improve access via keyboard:
  • Navigate between the images in a phone detail.
  • Add headings elements.
  • Add information about checkmarks in the phone detail specs.

By the way, do I have to modify the document in the full tutorial?

@felixzapata
Copy link
Author

hi @gkalpak, ¿is there any way that the use of the debounce option for the ng-model works with Protractor? I need this option for my changes. But with that, the e2e tests fail.

I know that there is an issue about that but nothing more.

@gkalpak
Copy link
Member

gkalpak commented May 30, 2016

Without having really looked into that, I can't imagine how protractor can be ignoring the debounce. It's something that Angular does and PRotractor doesn't have a way to "ignore" it.

What is probably happening, is that Protractor waits for the view to "stabilize" (which in Angular terms includes waiting for registered timeouts, such as the one used by debounce) and only then assets your expectation.

If you want to circumvent that, you can turn on ignoreSynchronization (read more about it here).

If you can't get it to work, post the failing test and I'll take a look.

@felixzapata
Copy link
Author

I think that with ignoreSynchronization does not work. I saw yesterday this information about timeouts. One of the test that fails is:

    it('should filter the phone list as a user types into the search box', function() {
      var phoneList = element.all(by.repeater('phone in $ctrl.phones'));
      var query = element(by.model('$ctrl.query'));

      expect(phoneList.count()).toBe(20);

      query.sendKeys('nexus');
      expect(phoneList.count()).toBe(1);

      query.clear();
      query.sendKeys('motorola');
      expect(phoneList.count()).toBe(8);
    });

The problem is the expect runs before the debounce finishes, so the results never will be the correct. It Is the same example that an user reported in the Protractor issue that I mentioned before.

I have tried using wait and setTimeout but I obtain the same errors.

@felixzapata
Copy link
Author

@gkalpak After a review, I think that my problem is related with some changes I've made in the repeater. I will check it.

@felixzapata
Copy link
Author

felixzapata commented May 31, 2016

@gkalpak done. Now the e2e tests pass in my local environment. I've added an browser.sleep suggested by @mohitjee15 and I've changed how to obtain the repeater in the scenario due to my changes in the template.

But here, Travis throws an error WebDriverError: unknown error: Chrome version must be >= 46.0.2490.0

@gkalpak
Copy link
Member

gkalpak commented May 31, 2016

Thx 👍
I will take a look this week !

@felixzapata
Copy link
Author

ok @gkalpak. Do I have to modify the full tutorial and send another pull request or I have to wait until this pull request is accepted?

@gkalpak
Copy link
Member

gkalpak commented May 31, 2016

Yes, you have to update the tutorial context as well, so we can merge them together.

@felixzapata
Copy link
Author

ok, I will work in the tutorial and I will send another pulll request as soon as possible.

@gkalpak
Copy link
Member

gkalpak commented Jun 3, 2016

@felixzapata, I'll review this together with the tutorial content PR - feel free to ping me when it's ready.
Thx for working on this 👍

@felixzapata
Copy link
Author

@gkalpak I've created the pull request with the new step in the tutorial.

About this pull request, please feel free to comment if I need to review the code in the new directives due to some mistakes.

@gkalpak
Copy link
Member

gkalpak commented Jun 15, 2016

Awesome! I will take a look soon.

@gkalpak
Copy link
Member

gkalpak commented Jun 20, 2016

Note to self: We need a new tag for this step.

@gkalpak
Copy link
Member

gkalpak commented Jun 20, 2016

Why had the previous steps be modified? Since this is an extra step at the end, previous steps should not be touched as far as I understand. Do I miss something?

@felixzapata
Copy link
Author

@gkalpak I will check again the pull request. I'm only want to upload three commits and I though that it was ok.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@felixzapata
Copy link
Author

@gkalpak done, a problem with the rebase

IgorMinar and others added 3 commits November 21, 2016 12:34
- Add the 'angular.js' script.
- Add the `ngApp` directive to bootstrap the application.
- Add a simple template with an expression.
- Add a stylesheet file ('app/app.css').
- Add a static list with two phones.
- Convert the static phone list to dynamic by:
  - Creating a `PhoneListController` controller.
  - Extracting the data from HTML into the controller as an in-memory dataset.
  - Converting the static document into a template with the use of the `ngRepeat` directive.
- Add a simple unit test for the `PhoneListController` controller to show how to write tests and
  run them using Karma (see README.md for instructions).
gkalpak and others added 12 commits November 21, 2016 12:34
- Introduce components.
- Combine the controller and the template into a reusable, isolated `phoneList` component.
- Refactor the application and tests to use the `phoneList` component.
- Refactor the layout of files and directories, applying best practices and techniques that will
  make the application easier to maintain and expand in the future:
  - Put each entity in its own file.
  - Organize code by feature area (instead of by function).
  - Split code into modules that other modules can depend on.
  - Use external templates in `.html` files (instead of inline HTML strings).
- Add a search box to demonstrate:
  - How the data-binding works on input fields.
  - How to use the `filter` filter.
  - How `ngRepeat` automatically shrinks and grows the number of phones in the view.
- Add an end-to-end test to:
  - Show how end-to-end tests are written and used.
  - Prove that the search box and the repeater are correctly wired together.
- Add an `age` property to the phone model.
- Add a drop-down menu to control the phone list order.
- Override the default order value in controller.
- Add unit and end-to-end tests for this feature.

Closes angular#213
- Replace the in-memory dataset with data loaded from the server (in the form of a static
  'phone.json' file to keep the tutorial backend agnostic):
  - The JSON data is loaded using the `$http` service.
- Demonstrate the use of `services` and `dependency injection` (DI):
  - `$http` is injected into the controller through DI.
  - Introduce DI annotation methods: `.$inject` and inline array

Closes angular#207
- Add a phone image and links to phone pages.
- Add an end-to-end test that verifies the phone links.
- Tweak the CSS to style the page just a notch.
- Introduce the `$route` service, which allows binding URLs to views for routing and deep-linking:
  - Add the `ngRoute` module as a dependency.
  - Configure routes for the application.
  - Use the `ngView` directive in 'index.html'.
- Create a phone list route (`/phones`):
  - Map `/phones` to the existing `phoneList` component.
- Create a phone detail route (`/phones/:phoneId`):
  - Map `/phones/:phoneId` to a new `phoneDetail` component.
  - Create a dummy `phoneDetail` component, which displays the selected phone ID.
  - Pass the `phoneId` parameter to the component's controller via `$routeParams`.
- Implement fetching data for the selected phone and rendering to the view:
  - Use `$http` in `PhoneDetailController` to fetch the phone details from a JSON file.
  - Create the template for the detail view.
- Add CSS styles to make the phone detail page look "pretty-ish".
- Implement a custom `checkmark` filter.
- Update the `phoneDetail` template to use the `checkmark` filter.
- Add a unit test for the `checkmark` filter.
- Make the thumbnail images in the phone detail view clickable:
  - Introduce a `mainImageUrl` property on `PhoneDetailController`.
  - Implement the `setImage()` method for changing the main image.
  - Use `ngClick` on the thumbnails to register a handler that changes the main image.
  - Add an end-to-end test for this feature.
- Replace `$http` with `$resource`.
- Create a custom `Phone` service that represents the RESTful client.
- Use a custom Jasmine equality tester in unit tests to ignore irrelevant properties.
- Add animations to the application:
  - Animate changes to the phone list, adding, removing and reordering phones with `ngRepeat`.
  - Animate view transitions with `ngView`.
  - Animate changes to the main phone image in the phone detail view.
- Showcase three different kinds of animations:
  - CSS transition animations.
  - CSS keyframe animations.
  - JavaScript-based animations.
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

I started reviewing this today, but something seems to have gone wrong (with rebasing maybe). Some unrelated changes or changes from other steps appear as part of the PR, which makes it very difficult to review.

@felixzapata, can you please rebase this on top of the current master and clean it up (e.g. merge all commits into one)?

"protractor": "^3.2.2"
"protractor": "^3.2.2",
"protractor-accessibility-plugin": "^0.1.1",
"shelljs": "^0.6.0"
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? It doesn't seem to be used.

README.md Outdated

### step-15 _Accessibility (a11y)_

- Add ngAria module.
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you end up removing it in the end? (Although it does seem strange to have a chapter about ARIA that doesn't mention or need ngAria 😃)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, finally I removed the module so I will update the README file.

@felixzapata
Copy link
Author

@gkalpak I´ve updated the PR to avoid the multiple commits.

I will take a look to your comments.

@felixzapata
Copy link
Author

@gkalpak I will update the PR with only one single commit.

@felixzapata
Copy link
Author

@gkalpak is there a problem inside Travis with the driver for Chrome?

@gkalpak
Copy link
Member

gkalpak commented Dec 14, 2016

Not sure, but if the tests pass locally, don't worry about Travis. I'll deal with it later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants