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

Naive solution to embedding iframes into example pages #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evmiguel
Copy link
Collaborator

@evmiguel evmiguel commented Dec 15, 2021

Proof of Concept

This PR is part of this Asana task. It is a naive solution to embedding iframes from ARIA-AT app to the WAI ARIA Practices site. Some assumptions and shortcuts taken for this proof of concept are:

  • This solution only interacts with the ARIA-AT app in production. This solution doesn't work with staging or local data because the iframe source comes from the data queried in production.
  • I've assumed that the latest git SHA in the Reports page takes precedence over older reports
  • To find examples with reports, I've assumed that the test plan id should match the example file names. This isn't the most accurate way to match examples with reports because the file names and test plan ids are not always identical.
  • There are inefficiencies with nested loops on lines 88-94, which makes the pre-build script run slower.

Open questions

  • Would users prefer to embed the reports manually with a link or have reports automatically embedded like in this POC?
  • Would users like a preview of the reports before finally embedding them?

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Proof that Alex saw the ARIA-AT App embedded inside the redesign for the disclosure navigation menu example

As you can see in the screenshot, I got the report to appear!

It's a POC so we're basically just proving that it can work, so I don't have a ton of feedback, because it clearly works. One thing I do think we should fix is just the fact that the axios dependency is not in the package.json. I think it worked for you because axios is installed globally? I had to add it for it to work for me.

Okay, and then the assumptions you've listed out.

  • It's true that we're assuming that the data comes from production. This doesn't bother me actually. I think it would be conceptually simpler to completely ignore the ARIA-AT App staging environments and pretend they don't exist.
  • The assumption about git shas seems right. I do think there is a slightly cleaner way to write the graphql query for when we implement this for real, but also there is an opportunity to clean up the query even more just by adding a couple fields to the API.
  • Instead of using the TestPlan id, perhaps in the future we can use the exampleURL which is in the metadata object.
  • I don't think this is inefficient! The main thing that might cause bad performance would be if we were making network requests per example, and fortunately you sidestepped that and did one global request for all examples. Nice.

Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Looks good!
In addition to @alflennik comments and given that this is a POC - for the future, I think it would be worth exploring aria-at-app providing a basic version of the Results + Complete Results tables made specifically for an embed such as this.

@@ -16,6 +16,11 @@ const loadExample = async (filePath, { exampleRelativeDirectory }) => {

const { title, head, body, outline, relatedLinks } = getContent();

const testReport = reportId ? `<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this should be a <section>

<div class="sidebar-right">${body}</div>
<div class="sidebar-right">
${body}
${testReport}
Copy link
Contributor

Choose a reason for hiding this comment

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

This conditionally appends the Test Report section to the body after the navigation link back to the design pattern. It seems to me that we'd want to preserve that always showing up at the end in the future

@@ -36,10 +41,14 @@ const loadExample = async (filePath, { exampleRelativeDirectory }) => {
`;
})
.join(" ")}
<li><a href="#test-report">Test Report</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be conditionally rendered as well or the Test Report menu link will show in cases where the Test Report section hasn't been rendered

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my note Erika! I think the plan, if I understand it right, is that we'll keep this unmerged, since it's a POC, but each of us will approve it once we verify you've addressed our comments.

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