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

All Example Pages: Add source copy instructions and CodePen button to HTML source section #3041

Merged

Conversation

ariellalgilmore
Copy link
Contributor

@ariellalgilmore ariellalgilmore commented Jun 11, 2024

Resolve issue #3027 with the following changes:

  1. Direct readers to copy HTML source from codepen instead of from the example page.
  2. Add an additional codepen button adjacent to the directions.

Preview

Preview revised combobox example page in compare branch

Review checklist

Reviewers: To learn what needs to be covered by each review, Follow the link for the type of review to which you are assigned.


WAI Preview Link (Last built on Tue, 23 Jul 2024 14:44:48 GMT).

@ariellalgilmore
Copy link
Contributor Author

@mcking65 this is not ideal code, but I tweaked some files and updated, so that the deploy preview now shows the open in codePen button inside the HTML source code section for this example only! Just wanted to get some feedback first before making more permenent changes

@mcking65
Copy link
Contributor

mcking65 commented Jun 25, 2024

@ariellalgilmore

I made some changes. My understanding of the discussion is that we keep the display but just make the changes I described in the revised top comment in this PR.

I'd like to suggest just one modification to the DOM order. Currently the open in codepen button in the HTML source section is appearing after the screen reader separator <div role="separator".... It would be better if it were placed between the paragraph and the separator like in the example section where the button is between the heading and the separator.

@mcking65 mcking65 changed the title Fix filtering error when copying white space from combobox autocomplete All Example Pages: Add source copy instructions and CodePen button to HTML source section Jun 25, 2024
@ariellalgilmore
Copy link
Contributor Author

Thanks @mcking65 for the updates! Keeping the html source code with the button looks good. I updated the dom order so that the button goes after the paragraph but before the separator

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Change to HTML source section on example pages.

The full IRC log of that discussion <jugglinmike> Topic: Change to HTML source section on example pages
<jugglinmike> github: https://github.com//pull/3041
<jugglinmike> Matt_King: Right now, this pull request is changing a single example page
<jugglinmike> Matt_King: But if folks agree with this direction, then we'd like to make the change to every single page
<jugglinmike> arigilmore: I updated the JavaScript to inject an "Open in Codepen" to an element with a certain HTML ID. All we'll need to do to add the change to other pages is add that HTML ID to each of the HTML files.
<jugglinmike> Matt_King: Is the button positioned similar to how it's positioned in the "example" section?
<jugglinmike> Matt_King: Yeah, it follows the paragraph that describes the section, and it precedes the rendering of the source code
<jugglinmike> s/Matt_King/arigilmore/
<jugglinmike> Matt_King: And does it behave appropriately for alternate viewport sizes?
<jugglinmike> arigilmore: Yes
<jugglinmike> Jem: Do we need to have a distinguishable button text for these two buttons?
<jugglinmike> Matt_King: No, because they both do the exact same thing, so it's important that they have the same label
<jugglinmike> Jem: Ah, I understand, now (I thought the behavior of the second button was special for the HTML code)
<jugglinmike> Matt_King: Before we make the change on 59 other example pages, does this look good?
<jugglinmike> Jem: Yeah!
<jugglinmike> Matt_King: Okay, then I think we're good to go
<jugglinmike> Matt_King: There are 3 pages (e.g. listbox, layout grid, and data grid) that have three examples on the same page, so pay special attention to those ones
<jugglinmike> arigilmore: Got it
<jugglinmike> Matt_King: By the way, I've been wanting to refactor those so there is only one example per page, but that involves changing some of the prose, so it's taking me some time
<jugglinmike> Matt_King: Anyway, this is one of those ones that we don't want to sit around for too long because it touches so many files

@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Change to HTML source section on example pages.

The full IRC log of that discussion <jugglinmike> Topic: Change to HTML source section on example pages
<jugglinmike> github: https://github.com//pull/3041
<jugglinmike> Matt_King: I would love to merge this very soon because it changes 59 pages, and I want to avoid conflicts with other pull requests
<jugglinmike> Matt_King: I think we need some people to look at some of these pages and just look for possible anomalies
<jugglinmike> Matt_King: Basically, right now, I think we need two reviewers in addition to myself
<jugglinmike> Matt_King: It's the same change on every page, but as we know, there are some variations in the example pages, e.g. the list box page and the button page
<jugglinmike> Matt_King: But we also want to pay attention to the more normal pages which only have one example per page
<jugglinmike> Matt_King: We also want to be sure that it works when you zoom in, and that it all looks acceptable on mobile
<jugglinmike> CurtBellew: I can help out
<jugglinmike> Jem: Me, too
<jugglinmike> Matt_King: Okay, could we maybe split the pages alphabetically? Jem, you could look at pages belonging the first half of the alphabet, and CurtBellew, you would look at pages in the second half...?
<jugglinmike> Matt_King: To be clear, you wouldn't have to look at every single page assigned to you; just use that to inform your sampling
<jugglinmike> s/use that/use that constraint/
<jugglinmike> Jem: I can take a look by the end of this week
<jugglinmike> CurtBellew: Same for me
<jugglinmike> Matt_King: Okay. I'm going to look at the coverage to make sure we didn't skip any examples
<jugglinmike> Matt_King: Don't worry about the "Feed" example--we don't have an "Open in CodePen" button there due to a technical shortcoming
<jugglinmike> Matt_King: This was a cool change. It was a tiny issue that someone raised, and I'm happy we made this decision

@a11ydoer
Copy link
Contributor

a11ydoer commented Jul 6, 2024

@mcking65 @curtbellew I did QA for "open in codepen" for HTML source section. Everything looks good!

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

This is approved!

@curtbellew
Copy link

I reviewed and only found one issue. In the single select listbox when I tried the first 'Open In Codepen' at the bottom, I am sent to codepen but the pen is empty. https://deploy-preview-328--aria-practices.netlify.app/aria/apg/patterns/listbox/examples/listbox-rearrangeable/#sc1_label

@ariellalgilmore
Copy link
Contributor Author

Thanks @curtbellew ! I fixed to confirm all example pages with multiple examples should be working now

@mcking65
Copy link
Contributor

@ariellalgilmore

I just checked the preview of Example Listboxes with Rearrangeable Options | APG | WAI | W3C. All the codepen buttons except for one work correctly. The one that produces an empty codepen is the codepen button for the HTML source section for example 1 that @curtbellew mentioned.

screenshot of codepen button

screenshot of empty codepen

@ariellalgilmore
Copy link
Contributor Author

Hi @mcking65 i just retriggered the deploy preview and the codepen should be working now: https://deploy-preview-328--aria-practices.netlify.app/aria/apg/patterns/listbox/examples/listbox-rearrangeable/#htmlsourcecode.

Screenshot 2024-07-23 at 7 55 03 AM

Copy link

@curtbellew curtbellew left a comment

Choose a reason for hiding this comment

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

Looks great!

@mcking65 mcking65 merged commit b28a984 into w3c:main Jul 27, 2024
28 checks passed
@mcking65
Copy link
Contributor

Thank you @ariellalgilmore for all your work on this! Once again, it turned into something bigger than anticipated and you rose to the occasion!

Thank you also to @curtbellew and @a11ydoer for your help reviewing all the pages effected!

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.

5 participants