Conversation
This will hold our Accessibility Conformance Report, which gets automatically generated from the OpenACR editor. This is a marketing/landing page that embeds that page, restyles it a bit, and provides it (and a .yaml version) for download. Our ACR is not done yet, just a placeholder. The marketing content on this page is also just a placeholder for now.
WalkthroughThe pull request introduces a new Accessibility Conformance Report (ACR) feature to the application. This involves creating a new route for the Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant VueRouter
participant ACRView
User->>Router: Navigate to /acr
Router->>VueRouter: Route request
VueRouter->>ACRView: Load ACRView component
ACRView-->>User: Render Accessibility Conformance Report
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/views/acr/ACRView.vue (2)
1-14: Add TypeScript and enhance component structure.The component structure could be improved for better maintainability and type safety:
- Consider using TypeScript for better type safety
- Add props validation for any incoming props
- Document the component's purpose using JSDoc
- Define required component imports
<script> +import { defineComponent } from 'vue' + +/** + * @component ACR + * @description Displays the Accessibility Conformance Report (ACR) and related information + */ +export default defineComponent({ -export default { name: 'ACR', components: { + // Add required components }, - data: () => ({ - }), + data: () => ({ + // Document the purpose of each data property + loading: false + }), methods: { + // Document each method's purpose + onClickSalesCTA() { + // Implementation + } }, -} +}) </script>
233-233: Remove trailing space.Fix the eslint warning by removing the trailing space:
- img.logo-image { + img.logo-image {🧰 Tools
🪛 eslint
[error] 233-233: Trailing spaces not allowed.
(no-trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/assets/acr/acr.css(1 hunks)app/core/Router.js(1 hunks)app/core/vueRouter.js(1 hunks)app/views/acr/ACRView.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/assets/acr/acr.css
🧰 Additional context used
🪛 eslint
app/views/acr/ACRView.vue
[error] 233-233: Trailing spaces not allowed.
(no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (2)
app/core/vueRouter.js (1)
459-462: LGTM! Route configuration follows best practices.The new route is properly configured with:
- Correct path definition
- Lazy loading using dynamic import
- Consistent naming convention for the chunk
app/core/Router.js (1)
564-565: LGTM! Backbone route properly configured.The new route is correctly added using the SingletonAppVueComponentView pattern, consistent with other Vue-based routes in the application.
app/views/acr/ACRView.vue
Outdated
| .text-center | ||
| a.btn.btn-primary.btn-large.btn-moon.sales-btn(href="https://docs.google.com/presentation/d/1KgFOg2tqbKEH8qNwIBdmK2QbHvTsxnW_Xo7LvjPsxwE/edit?usp=sharing" target="_blank") Sample Lesson | ||
| a.btn.btn-primary.btn-large.btn-moon.sales-btn(@click="onClickSalesCTA") Request a Quote |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA labels to buttons.
The buttons need proper ARIA labels for better accessibility:
- a.btn.btn-primary.btn-large.btn-moon.sales-btn(href="https://docs.google.com/presentation/d/1KgFOg2tqbKEH8qNwIBdmK2QbHvTsxnW_Xo7LvjPsxwE/edit?usp=sharing" target="_blank") Sample Lesson
- a.btn.btn-primary.btn-large.btn-moon.sales-btn(@click="onClickSalesCTA") Request a Quote
+ a.btn.btn-primary.btn-large.btn-moon.sales-btn(
+ href="https://docs.google.com/presentation/d/1KgFOg2tqbKEH8qNwIBdmK2QbHvTsxnW_Xo7LvjPsxwE/edit?usp=sharing"
+ target="_blank"
+ aria-label="View sample lesson in new tab"
+ ) Sample Lesson
+ a.btn.btn-primary.btn-large.btn-moon.sales-btn(
+ @click="onClickSalesCTA"
+ role="button"
+ aria-label="Request a quote for our services"
+ ) Request a Quote📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .text-center | |
| a.btn.btn-primary.btn-large.btn-moon.sales-btn(href="https://docs.google.com/presentation/d/1KgFOg2tqbKEH8qNwIBdmK2QbHvTsxnW_Xo7LvjPsxwE/edit?usp=sharing" target="_blank") Sample Lesson | |
| a.btn.btn-primary.btn-large.btn-moon.sales-btn(@click="onClickSalesCTA") Request a Quote | |
| .text-center | |
| a.btn.btn-primary.btn-large.btn-moon.sales-btn( | |
| href="https://docs.google.com/presentation/d/1KgFOg2tqbKEH8qNwIBdmK2QbHvTsxnW_Xo7LvjPsxwE/edit?usp=sharing" | |
| target="_blank" | |
| aria-label="View sample lesson in new tab" | |
| ) Sample Lesson | |
| a.btn.btn-primary.btn-large.btn-moon.sales-btn( | |
| @click="onClickSalesCTA" | |
| role="button" | |
| aria-label="Request a quote for our services" | |
| ) Request a Quote |
app/views/acr/ACRView.vue
Outdated
| section#acr-report | ||
| h1.heading-corner Full Report | ||
| iframe#acr-iframe(src="/acr/acr.html" frameborder="0" scrolling="no" onload="this.style.height = this.contentWindow.document.documentElement.scrollHeight + 'px';") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance iframe accessibility.
The iframe needs a title and better height management:
- iframe#acr-iframe(src="/acr/acr.html" frameborder="0" scrolling="no" onload="this.style.height = this.contentWindow.document.documentElement.scrollHeight + 'px';")
+ iframe#acr-iframe(
+ src="/acr/acr.html"
+ title="Accessibility Conformance Report"
+ frameborder="0"
+ scrolling="no"
+ @load="adjustIframeHeight"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| section#acr-report | |
| h1.heading-corner Full Report | |
| iframe#acr-iframe(src="/acr/acr.html" frameborder="0" scrolling="no" onload="this.style.height = this.contentWindow.document.documentElement.scrollHeight + 'px';") | |
| section#acr-report | |
| h1.heading-corner Full Report | |
| iframe#acr-iframe( | |
| src="/acr/acr.html" | |
| title="Accessibility Conformance Report" | |
| frameborder="0" | |
| scrolling="no" | |
| @load="adjustIframeHeight" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/views/acr/ACRView.vue (1)
36-36: 🛠️ Refactor suggestionEnhance iframe height management.
The current inline
onloadhandler for managing iframe height should be moved to a proper Vue method for better maintainability and error handling.Apply this diff to improve the iframe implementation:
- iframe#acr-iframe(src="/acr/acr.html" frameborder="0" scrolling="no" onload="this.style.height = this.contentWindow.document.documentElement.scrollHeight + 'px';" title="Accessibility Conformance Report") + iframe#acr-iframe( + src="/acr/acr.html" + title="Accessibility Conformance Report" + frameborder="0" + scrolling="no" + @load="adjustIframeHeight" + )Add the following method to handle iframe height adjustment:
methods: { adjustIframeHeight(event) { try { const iframe = event.target; iframe.style.height = `${iframe.contentWindow.document.documentElement.scrollHeight}px`; } catch (error) { console.error('Error adjusting iframe height:', error); } } }
🧹 Nitpick comments (2)
app/views/acr/ACRView.vue (2)
4-12: Remove empty component structure if not needed.The component contains empty
components,data, andmethodssections. If these aren't needed, they should be removed for better code maintainability.Apply this diff to simplify the component structure:
export default { name: 'ACR', - components: { - }, - - data: () => ({ - }), - - methods: { - }, }
185-185: Remove trailing whitespace.There is a trailing whitespace after
img.logo-image.Apply this diff to fix the whitespace:
- img.logo-image { + img.logo-image {🧰 Tools
🪛 ESLint
[error] 185-185: Trailing spaces not allowed.
(no-trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/common/Footer.vue(1 hunks)app/components/common/Navigation.vue(3 hunks)app/locale/en.js(2 hunks)app/views/acr/ACRView.vue(1 hunks)app/views/schools/PageSchools.vue(1 hunks)
🧰 Additional context used
🪛 ESLint
app/views/acr/ACRView.vue
[error] 185-185: Trailing spaces not allowed.
(no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (6)
app/locale/en.js (3)
439-439: LGTM: Added new navigation item for accessibilityThe addition of the accessibility navigation item with a link to ACR/VPAT is a good practice for improving site accessibility and compliance information.
5653-5655: LGTM: Added accessibility ACR prefix, link text and suffixThe addition of these accessibility-related strings provides clear information about WCAG 2.1 Level AA and Revised Section 508 Standards compliance.
5653-5655: Verify accessibility link functionalityLet's verify that the ACR/VPAT link is properly implemented and accessible.
app/components/common/Footer.vue (1)
104-104: LGTM!The addition of the accessibility link in the footer with country-based filtering is well implemented.
app/views/schools/PageSchools.vue (1)
435-441: LGTM!The addition of the accessibility information and ACR link is well integrated into the schools page.
app/components/common/Navigation.vue (1)
38-38: LGTM!Good improvements:
- Added accessibility navigation item
- Fixed typo in variable name (
teacherCocoCllasses→teacherCocoClasses)Also applies to: 302-302, 312-312
2 participants
Adds the accessibility compliance report. Landing page at /acr, embeds the HTML version output from the ACR tool, also links to HTML and YAML downloads of same. Also adds a link to the accessibility description in /schools and adds it to the footer for US users.
Page
Schools Page Link
Footer link
This is just for US users, as these regulations are mostly relevant here, and the ACR itself isn't localized. Non-US users would still see it from /schools page or direct link, but we probably don't need in the footer.
Summary by CodeRabbit
New Features
Style
Documentation