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

disclosure navigation Example: Add "Open in CodePen" button #1651

Merged
merged 1 commit into from
Oct 31, 2021

Conversation

spectranaut
Copy link
Contributor

@spectranaut spectranaut commented Dec 3, 2020

Hi everyone! This "Open to codepen" is in it's own PR because I'm not totally sure how to move forward with it.

Here is example: Preview Disclosure Navigation Example

The problem is that when we open these examples in codepen, the links send you back to the APG page instead of just updating the main content within the copepen HTML rendering. The reason why is that we send a base url to Codepen so that all images and links in the HTML lead back to the APG website.

The codepen example works up until you click a link, so it is probably useful to most programmers. My suggestion is that we leave it in this state for now, and see if there is an elegant solution to keep the example links working in codepen when the example is updated to our APG coding guidelines (this example hasn't been updated yet).

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2020

Regression test coverage:

Examples without any regression tests:

  • dialog-modal/alertdialog.html

Examples missing some regression tests:

  • dialog-modal/datepicker-dialog.html:
    • textbox-aria-describedby
  • menu-button/menu-button-actions-active-descendant.html:
    • menu-up-arrow
    • menu-down-arrow
    • menu-character
  • toolbar/toolbar.html:
    • toolbar-tab
    • toolbar-right-arrow
    • toolbar-left-arrow
    • toolbar-home
    • toolbar-end
    • toolbar-toggle-esc
    • toolbar-toggle-enter-or-space
    • toolbar-radio-enter-or-space
    • toolbar-radio-down-arrow
    • toolbar-radio-up-arrow
    • toolbar-button-enter-or-space
    • toolbar-menubutton-enter-or-space-or-down-or-up
    • toolbar-menu-enter-or-space
    • toolbar-menu-down-arrow
    • toolbar-menu-up-arrow
    • toolbar-menu-escape
    • toolbar-spinbutton-down-arrow
    • toolbar-spinbutton-up-arrow
    • toolbar-spinbutton-page-down
    • toolbar-spinbutton-page-up
    • toolbar-checkbox-space
    • toolbar-link-enter-or-space
    • toolbar-spinbutton-role

Example pages with Keyboard or Attribute table rows that do not have data-test-ids:

  • dialog-modal/alertdialog.html
    • "Keyboard Support" table(s):
      • Tab
      • Shift + Tab
      • Escape
      • Command + S
      • Control + S
    • "Attributes" table(s):
      • alertdialog
      • aria-labelledby=IDREF
      • aria-describedby=IDREF
      • aria-modal=true
      • alert

SUMMARY:

55 example pages found.
1 example pages have no regression tests.
3 example pages are missing approximately 27 out of approximately 780 tests.

ERROR - missing tests:

Please write missing tests for this report to pass.

@carmacleod
Copy link
Contributor

Hmm, I see what you mean.

I guess the Menubar navigation has the same problem - sorry I didn't notice earlier.

Treeview navigation will have the problem, too - currently being reviewed in #1558.

I wonder if we should maybe leave the codepen button off of the navigation examples until this problem is fixed?

How might updating to the latest APG coding guidelines help?

Also, curious whether we would still need CodePen's base url to point back to the APG site if we inline SVG's? Not many of the examples have a non-inlined image, except for the map in the disclosure example, maybe? What links still need to go back to the APG site?

@spectranaut
Copy link
Contributor Author

spectranaut commented Dec 4, 2020

Thanks for the thoughts @carmacleod !

How might updating to the latest APG coding guidelines help?

We definitely need an item about updating the APG coding guidelines to include things you can and can't do in order to open the example in codepen! But I'm really not sure the technical solution for these navigational menus. I haven't had a moment to really put on my thinking cap about it, however.

Also, curious whether we would still need CodePen's base url to point back to the APG site if we inline SVG's? Not many of the examples have a non-inlined image, except for the map in the disclosure example, maybe? What links still need to go back to the APG site?

Unfortunately, we really do need the base url, for every other image that is referenced by a relative path in the HTML. Take the carousel examples, or the the disclosure image -- these images are pulled into the codepen directly from the APG when the codepen renders. It's just relative URLS from the CSS that don't work (they don't know about the base url). This makes me think we should maybe just leave all reference to SVGs in the HTML.

@carmacleod
Copy link
Contributor

Right, forgot about the carousel images.

Are there any examples that need the base url to point to APG site for links?
Just trying to understand the scope of the problem. :)

The Menu Button example uses absolute URLs, so it's ok (hmm, but the codepen button in that example doesn't have the right style).

I guess the Breadcrumb Example uses relative links into the APG, so it currently needs the base url to be APG, but the links can probably be changed to absolute URLs.

Can't think of any others, but I didn't look deeply.

@spectranaut
Copy link
Contributor Author

i just looked through and your right, I don't think there are any relative links that are essential to the examples, so it is just the images.

I also just tried to remove the "base" url override locally to test if the example would work in codepen, and it didn't -- but it turns out it didn't because the "onclick" handler is never replaced because of an error! I just pushed a fix for that. With that fix, the example does work if you remove the "base" reference. So one option for a fix is to consider changing the way we bring in images and removing base to get the examples that use fake links to work.

But I also thought of another option -- if we are replacing the default behavior for activating a link in this example anyway, we could just call event.preventDefault(). When you add event.preventDefault() right now, though, the url fragment does not update and the menu does not close, so we would have to add that behavior as well. What do you think of this option?

cspell.json Outdated Show resolved Hide resolved
cspell.json Outdated Show resolved Hide resolved
@carmacleod
Copy link
Contributor

But I also thought of another option -- if we are replacing the default behavior for activating a link in this example anyway, we could just call event.preventDefault(). When you add event.preventDefault() right now, though, the url fragment does not update and the menu does not close, so we would have to add that behavior as well. What do you think of this option?

Interesting thought, @spectranaut. I will have to defer to @smhigley on that, because she wrote DisclosureNav.
Sarah, please read the previous comments to understand the back story for Valerie's question. :)

@spectranaut
Copy link
Contributor Author

Resolved the conflicts!

@mcking65 mcking65 changed the title Open in codepen: add to disclosure navigation disclosure navigation Example: Add "Open in CodePen" button Jan 19, 2021
@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern labels Jan 19, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Jan 19, 2021
Base automatically changed from master to main March 3, 2021 18:13
@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2021

So one option for a fix is to consider changing the way we bring in images and removing base to get the examples that use fake links to work.

I think I prefer this option if it can work for all examples. base elements are a bit confusing in my opinion, and adding code to work around the effect of the base element seems like it would add further complexity that distracts from what the example is actually about.

@zcorpan zcorpan force-pushed the codepen-disclosure-navigation branch from 3aade06 to 1a30872 Compare April 19, 2021 21:32
@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2021

However, looking at this particular example, the scrolling that happens by navigating to the fragment #mythical-page-content is a bit annoying and could be disorienting, so maybe it should preventDefault() anyway to avoid the scrolling.

@smhigley
Copy link
Contributor

@zcorpan this was discussed and resolved in the other disclosure nav example, I think it would make sense to adopt the same approach for this one: #1614 (comment)

@zcorpan zcorpan force-pushed the codepen-disclosure-navigation branch from 1a30872 to 3f207ae Compare April 19, 2021 21:44
@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2021

@smhigley OK, so use regular link and don't preventDefault()? Like it is currently in this PR?

@zcorpan
Copy link
Member

zcorpan commented Apr 19, 2021

I noticed that #1614 also added (or changed) the "fake link behavior" code that was almost identical to that in the second commit of this PR (1a30872), so I removed that commit here with a git rebase.

@mcking65 mcking65 added the agenda Include in upcoming Authoring Practices Task Force meeting label Apr 26, 2021
@zcorpan zcorpan requested a review from smhigley May 10, 2021 13:47
@zcorpan
Copy link
Member

zcorpan commented Oct 26, 2021

@mcking65 should this be merged?

@mcking65
Copy link
Contributor

Although minutes for October 19 meeting do not indicate a merge decision, my memory of the discussion is that the current behavior of the example when viewed in Codepen, while not ideal, is acceptable. So, I will merge.

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Approving based on memory of Octt 19, 2021 meeting discussion

@mcking65 mcking65 merged commit e250d12 into main Oct 31, 2021
@mcking65 mcking65 deleted the codepen-disclosure-navigation branch October 31, 2021 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda Include in upcoming Authoring Practices Task Force meeting enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants