-
Notifications
You must be signed in to change notification settings - Fork 9
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
Initial upload with build script #2
Conversation
CC: @bblaisATcoveo |
@Garneauma Peux-tu stp faire une revue? |
e3d1d39
to
5ffcf2c
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.
There is a few change request, see my inline comments
The following is more my regarding test that I would need todo later in a local instance:
- Execute the build script
- Test the user interface, at least for the following 3 state
- Result page
- No result page
- Error page
- Test the status message that is conform to WCAG 4.1.3
- Test the suggestion box which seem to show/hide automatically (See the how the js control it via the global variable
suggestionsElement
@GormFrank all the accessibility issue, the potential security issue, the licensing information and the versioning identification need to be fixed/addressed prior to merge this. Also I will didn't completed a functional testing through where the user are going to interact with this. I may have additional comment and change request after that. |
@duboisp Thank you for your review. Version identification can happen anytime BEFORE the first release, but not a blocker to the merging of this PR. Like I mentioned in my comments, the connector.js has been updated from the vendor side and therefore my version is obsolete. In the same direction, it means that we should focus on merging this PR as soon as possible, EVEN if it doesn't necessarily work on its own hence the "initial upload" mention in the title. I will tho tackle some of the changes requested in some other files of this PR. |
bc0c96c
to
06eca39
Compare
Comments that I have left open could be tackled later on, if you agree @duboisp. This is a Pull request targeted for version 1.0.0-alpha. Ready to be tested, merged and re-tested. Sent to Coveo for separate testing as well. (CC: @bblaisATcoveo) |
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.
There is a few accessibility issue to fix/address and I did suggested a few optimization.
I didn't review all my preceding comments yet and I didn't tested the code yet.
dcc3c34
to
8c6b3cf
Compare
Ready for your final review @duboisp |
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.
This code seem ok from an accessibility and security code review perspective. I also tested and interacted with a test pages. I did found issue and the recommended mitigation was applied.
There is a lot of items in regards of code quality and code performance. My understanding is those issue are going to be addressed later and coordinated by the PP technical advisor lead. Those optimisation are not a blocker for merging this PR.
Includes:
Still missing the following, for future PRs: