Skip to content
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

feat: add accessibility page #861

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Conversation

KristinAoki
Copy link
Member

@KristinAoki KristinAoki commented Mar 4, 2024

JIRA Ticket: TNL-10754

This PR adds the code for the Accessibility request page. This additional support the DEPR-381.

Testing

  1. Set SHOW_ACCESSIBILITY_PAGE in env.dev to true
  2. Navigate to http://localhost:2001/accessibility
  3. Compare the content to http://localhost:18010/accessibility
  4. Submit button should be disabled until all fields are valid
  5. Fill out the form and check that the email validation is correct and all fields are required
  6. Click Submit
  7. Should see success alert

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (3c661e1) to head (b37b986).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
+ Coverage   90.94%   91.01%   +0.06%     
==========================================
  Files         520      530      +10     
  Lines        9157     9226      +69     
  Branches     1920     1935      +15     
==========================================
+ Hits         8328     8397      +69     
  Misses        797      797              
  Partials       32       32              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

I realize this is just a draft, but I was curious. :)

},
a11yBodyIntroGraph: {
id: 'a11yBodyIntroGraph',
defaultMessage: `At edX, we seek to understand and respect the unique needs and perspectives of the edX global community.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: `At edX, we seek to understand and respect the unique needs and perspectives of the edX global community.
defaultMessage: `We seek to understand and respect the unique needs and perspectives of the global community.

Do you mind making sure there aren't any instances of edX being referenced specifically in the messages?

},
accessibilityPolicyFormSuccess: {
id: 'accessibilityPolicyFormSuccess',
defaultMessage: 'Thank you for contacting edX!',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'Thank you for contacting edX!',
defaultMessage: 'Thank you for contacting us!',

Another example of the above. (There are more, I just didn't tag them explicitly.)

Comment on lines +16 to +17
const communityAccessibilityLink = 'https://www.edx.org/accessibility';
const email = '[email protected]';
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine this will become configurable before the PR gets out of draft; just checking.

], 'Course Apps API service');

export const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL;
export const getZendeskrUrl = () => `${getApiBaseUrl()}/zendesk_proxy/v0`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can make Zendesk a plugin?

@KristinAoki
Copy link
Member Author

@arbrandes The scope of this work is to match what is already present in studio-frontend and edx-platform. So all the variable definitions and intl messages are copied over to this repo. So for now, no additional work will be made to make it site agnostic.

@jristau1984
Copy link
Contributor

@arbrandes The scope of this work is to match what is already present in studio-frontend and edx-platform. So all the variable definitions and intl messages are copied over to this repo. So for now, no additional work will be made to make it site agnostic.

That is correct. The goal here is to move items out of studio-frontend so it can be deprecated. However, these improvements are worth tracking, likely in an issue against the repo for future work. Thanks!

@arbrandes
Copy link
Contributor

In discussion with Axim engineering, the consensus is that since this is a port it's fine for it to go in as is. That said, a few important points were raised:

  • We should be intentional about the tech debt we're accruing, so yes, let's add issues to track it.
  • It's not clear that this feature makes sense for the community at large. So one of the specific issues would be to bring up with product whether it should be deprecated, and/or if it should be made pluggable.

@KristinAoki KristinAoki marked this pull request as ready for review March 5, 2024 17:00
@KristinAoki KristinAoki requested a review from a team as a code owner March 5, 2024 17:00
@KristinAoki KristinAoki merged commit f717cda into master Mar 5, 2024
6 checks passed
@KristinAoki KristinAoki deleted the KristinAoki/add-accessibility-page branch March 5, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants