-
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
Button Examples: Add Open in Codepen and inline SVG in HTML using SVG "def"s #1642
Conversation
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:55 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
Very cool, @spectranaut ! Nice work-around for :before and :after! Not sure why the codepens are untitled? Did you create this branch from slightly old code? :) Noticed that the menubar example needs some margin between it and the codepen button. Not sure why other examples have enough space? Changing the margin to have 5px on top (and bottom) fixes it, but I'm not sure if that's the best fix. .menubar-editor {
margin: 5px 0;
...
} |
65442c1
to
1bbf826
Compare
You are right @carmacleod I was on old code, I just rebased! Titles should be in now. I used this URL encorder: https://yoksel.github.io/url-encoder/ Also, do you think this is a ok direction to move forward with the APG? Essentially, we will have to require that a "image" folder is no longer used, and instead the options are to inline the SVG in the CSS or inline them in the HTML. |
Good question. I don't know enough about SVGs to know what people "usually" do. We need to be sure we don't break Windows HCM, which is why we converted some of the other examples over to inline SVGs in the HTML and made the SVGs use currentColor. I don't know if currentColor works for inline CSS content SVGs? It might. Unfortunately, the editor menubar icons don't yet work properly in high contrast mode (not because of this PR - I guess we didn't convert the SVGs over to be inline and use currentColor, and I see that the grey triangles and blue dots and checkmarks fail in High Contrast Black). We did fix the navigation menubar example though (inlined triangles that use currentColor), so maybe we can do something similar for editor menubar. (And then we can test whether currentColor works in CSS content url()'s or not). I think the button example is good to go! The SVG defs work nicely for that one. Thanks for rebasing - title working now. Did you want to split this into 2 PRs so button example can go in right away? It seems we have some discussion and/or further svg coding/testing still to do for the editor menubar example. |
Thanks for the suggestions @carmacleod ! I'll break this up into two PRs and use the navigational menubar as an example for the inline-css option instead. |
1bbf826
to
c40fe83
Compare
c40fe83
to
da266dc
Compare
da266dc
to
db0c693
Compare
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.
+1
Very nice, @spectranaut !
The mute off/on icons look great in both CodePen and High Contrast Mode (Black and White - tested in Edge).
@spectranaut |
Inlining in HTML
In #1534, @carmacleod suggested we use SVG defs to inline the SVGs into the HTML so they will load in codepen. See the code for the button example for an implementation of this suggestion.
Preview Button Example