-
Notifications
You must be signed in to change notification settings - Fork 355
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
Infrastructure: Add support for opening examples in CodePen #1110
Conversation
05e14d3
to
f812b89
Compare
f812b89
to
2480546
Compare
This was discussed in the September 10, 2019 meeting. There was consensus that a separate section with its own heading is too heavy and that it would be better if the button were positioned to the right of the example heading. I moved the button in the HTML, but I need someone to add the necessary styling to visually position it. I assume it would go in examples/css/core.css. @jnurthen also suggested a feature request to automatically run tidy on the HTML. @jnurthen, are you making this request because the HTML in our files still isn't tidy in some cases? Or, is it that the formatting in our files is lost in transmission to CodePen? |
@mcking65 the HTML all seemed to be indented by a whole bunch of extra spaces. I want those removed. |
@jnurthen commented:
Ah, thanks. Those are probably just the leading spaces that exist in our source. We should just trim the number of spaces from each line equal to the indent of the first line of the example. |
This additional white space is in our own source code. Basically, because the example code follows the indentation of the example page itself the example code has more white space. Similar to how this works for code in a Other than that this seems to work pretty well, thanks @spectranaut. |
@mcking65 if there’s an issue for that can you assign that to me?
|
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<carmacleod> TOPIC: open examples in CodePen<carmacleod> mck: need someone to do some css, then Jesse Beach will add to all of the examples <carmacleod> github: https://github.com//pull/1110 <carmacleod> zoe: I will make the button pretty |
I have committed and pushed some core CSS changes that styles this button and positions it next to the “Example” heading text. Please review. |
@ZoeBijl Please have a look at the grid page because we have longer headings |
The ARIA Authoring Practices (APG) Task Force just discussed The full IRC log of that discussion<carmacleod> TOPIC: Open examples in CodePen<carmacleod> github: https://github.com//pull/1110 <carmacleod> Zoe: button is blue - can make it purple <jamesn> +1 on the button - its awesome. I have no preference on the colour <carmacleod> Zoe: preview https://raw.githack.com/w3c/aria-practices/issue1102-open-example-codepen/examples/slider/multithumb-slider.html <carmacleod> james: the HTML has incorrect indentation when opened in CodePen <carmacleod> Zoe: that doesn't have to be fixed before merge - it can be fixed after |
@carmacleod thank you for the reminder ⭐️. The current CSS works with short and long headings. When you zoom in you’ll end up with something like this: @spectranaut have you tested this on a page with multiple examples? |
@ZoeBijl I remember designing the pattern with the possibility of multiple examples per page and I might have tested it locally when making this PR, but I didn't test it thoroughly. The function |
@@ -7,6 +7,7 @@ | |||
<!-- Core js and css shared by all examples; do not modify when using this template. --> | |||
<link rel="stylesheet" href="https://www.w3.org/StyleSheets/TR/2016/base.css"> | |||
<link rel="stylesheet" href="../css/core.css"> | |||
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can do this without jQuery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a lot more obnoxious to get the javascript and css resources but certainly we could. Why should we not use jQuery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird to include an entire library to open an example in an external resource 🤷♀. Is it a one time annoyance? Or is it annoying every time we add an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one time annoyance, to replace the $.get
ajaxes in the examples/js/examples.js
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't say that I won't to write that code, lol :) but I hear your point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can have a look 👩💻.
a5a69ea
to
d96dd7d
Compare
@mcking65 -- I added a commit to add the button in the However, you still have to edit the example page in three ways:
AND, unfortunately, the data grids example does not work when opened in codepen. The code to initialize the data grid expects the grid to be in the APG example page. When we role this out, we are going to have to test each example -- it it works, then we can add the buttons (by doing the three steps above), if it doesn't work, then we will have to make a backlog issue to refactor the code so that the example can work in isolation. We should revert the commit to add the buttons to dataGrid before merging this, I just wanted to show it as a proof of concept for now. |
@spectranaut, thank you. They are working beautifully! Before merging, let's reduce this PR so that it is only adding the capability but not changing any example pages, i.e., changes only examples.js and core.css, reverting d96dd7d and changes to spinbutton.
We should do these mods in a series of separate PRs.
Do we also have to load jquery on every page now as well? It would be useful to have some way of tracking implementation progress and then later enforcing these requirements. What do you think of an "allowed to fail" lint rule that tests to see if these conditions might not be present?
Bummer; I hope there aren't many such cases. |
Hi Matt, I just pushed a few commits:
Because of the last item, we should have one more code review on this PR, I think! I also added a how to page in the wiki. I didn't add anything to travis to check for whether the code pen button is on the page. Maybe we should keep a list of examples without CodePen links in the wiki page? |
8944220
to
625a43d
Compare
@spectranaut, this is looking great!! After merging, we can figure out how to track implementation in the example pages. @smhigley can you please take a look at the new implementation? If it looks good to you, we'll merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me!
The only thing I can think of is maybe it would be good to hide the "Open in Codepen" button until the requests have either completed or errored. I think we could just merge this now though and see how it goes, then maybe adjust in the future if desired :).
Same goes for the comments I left -- they were just thoughts I had when reading it through, not things that would need to be changed.
}, 500); | ||
|
||
setTimeout(() => { | ||
clearInterval(timerId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have a hideButton
call here too, with an error message about loading files timing out?
document.getElementById(jsonInputId).value = JSON.stringify(postJson); | ||
clearInterval(timerId); | ||
} | ||
}, 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a thing that needs to be fixed, but it looks like async/await is supported in all modern browsers. If we wanted to avoid using manual intervals, pushing all the request.send
promises into an array then doing await Promise.all
could work.
625a43d
to
6998933
Compare
This PR is a proof of concept for issue #1102.
Codepen has a POST API to prefill the HTML/CSS/JS inputs. This PR adds a dynamically created form that can send a POST request with the appropriate data to Codepen (look for "Open this example in Codepen" button on the
slider/multithumb-slider.html
page).I tried to do this in a generalized way -- the code is mostly in
examples/js/examples.js
-- so that each example page would only need a small amount of html/js added, most of which could be added/copied directly from the template file. This solution should also work for examples pages that contain multiple examples.Here is a PR for doing the same thing but using JSFiddle: #1111
Preview WIP
View the slider example with the codepen button