Add styles to the demo page #187
Conversation
✅ Deploy Preview for popover-polyfill ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@jgerigmeyer There's still obviously some style cleanup and refining left. I still need to:
|
|
@stacyk I still need to clean up the menu, and add the prism styles to the shadow root example, but I think this is ready for an initial style review. I'm most curious for you feedback on the header, getting that large text to grow and still fit as the layout widens. |
@dvdherron I'll check this out today! Update: I looked briefly today, I see the text changes sizes at a few breakpoints, but are we trying to minimize the gap between text and headline? I can look further tomorrow. |
Yeah, I think it's a matter of finding the right balance between the large text and the paragraphs that sit next to it. Like, do you see a way this could be handled better? |
|
@SondraE Looking at what's here now, would you still prefer that the header is sticky? And in the mockup, in the intro paragraph "displaying popover content" is a link. To where should that link lead? |
There was a problem hiding this comment.
@dvdherron let me know if that is sorta what you are going for. Left a few code update suggestions to allow the title to grow and fill the gap a bit.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
* main: v0.4.3 do not error if `window` is undefined Export injectStyles -- fixes #200
I bumped the line-height down a bit in 67ee7c4. If you think it needs to be taken down some more, I can adjust. |
There was a problem hiding this comment.
Assigning to @mirisuzanne for final style review. Note that Firefox 125 now supports popovers, so to test the polyfilled styles I needed to download FF 124 from https://download-installer.cdn.mozilla.net/pub/firefox/releases/124.0.2/mac/en-US/Firefox%20124.0.2.dmg
There was a problem hiding this comment.
A few small notes, but looking great. For me the cross-tree popover only worked in FF 124 with the polyfill, and not in my most recent Vivaldi.
I also wonder if we want some indicator at the top of the page that says if the polyfill is active or not? And if not, tells you what browser versions would need it.
css/base.css
Outdated
| } | ||
| } | ||
|
|
||
| :focus { |
There was a problem hiding this comment.
| :focus { | |
| :focus-visible { |
I think this is pretty safe at this point.
There was a problem hiding this comment.
changed in 27bd5a6
| <button popovertarget="popover11-1" popovertargetaction="show"> | ||
| Items | ||
| </button> | ||
| <a |
There was a problem hiding this comment.
Should we provide any indication that these are external links? Maybe use the box-with-an-arrow icon?
There was a problem hiding this comment.
@dvdherron I added these arrows in 63e0fa5, but feel free to adjust/replace/style them.
There was a problem hiding this comment.
@jgerigmeyer what you added looked great. I just adjust a few links for consistency.
Yeah, I've noticed that too. From what I can tell the polyfill is doing the right thing there, but browser support for that piece is lagging behind. whatwg/html#9109
I added a note and link in 63e0fa5 -- @dvdherron please make it look better 🙏 |
Polyfill not applied:
Polyfill applied:
|
Related Issue(s)
Fixes #168
Reminder to add related issue(s), if available.
Steps to test/reproduce
Please explain how to best reproduce the issue and/or test the changes locally (including the pages/URLs/views/states to review).
Show me
Provide screenshots/animated gifs/videos if necessary.