-
Notifications
You must be signed in to change notification settings - Fork 8
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
Release of stable v1.0.0 + Added documentation / content #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary review. See my requested change
I will do a separate review for connector.js
file because the diff is too large, do include a lot a changes where some seems considerable and the code are not readable in the diff mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completed the code review and it is mostly re-organized. I didn't completed a functional test considering that it should more likely work the same as the time of my previous review.
I did have a few change, see the inline comment.
Once those comment is addressed, I don't have any issue with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved once the change to the connector.js is removed.
The items that is commented is more for optimization which can be completed later.
3a82e6b
to
16e938d
Compare
1b9e8be
to
a8d2882
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to run a local test tomorrow morning, but prior to merge this:
- Fix the role override issue with the heading level 2.
- Check if the button is a submit or only executing a local action
- Provide some quick explanation/indication about how SC 4.1.3 is met. regarding "showing the number of results" (if that SC is applicable) See my inline comments.
I completed a very quick local testing. I noted the pagination button are not styled as expected: I confirm the button is not a submit button. So this need to be updated The focus to the search result work differently for the advanced search where the whole result get focus. I recommend to implement the same focusing behaviour. When there is "no search result" in the contextual search (search term "canadaa") there is no focus management like the other pages and I don't think the user will know the status change too. There is no role status and no focus move. |
Added to To do list. This is something that only affects local/GitHub testing based on the development environment; not a problem for deployment. No result focus fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good.
@GormFrank, did you ?
- Retest the "did you means" buttons to ensure it remains functional
- retest the focus when searching for the 6 test pages
Once those tests are run with a confirmation that it is working, you can proceed with the merge of this PR.
Confirmed tests conducted in Safari and Chrome. |
This is to bring the project to v1.0.0, which includes:
The following hidden page already has this code for testing purposes: https://www.canada.ca/en/service-canada/francis/srb.html