oddbird / popover-polyfill

Polyfills the HTML popover attribute and showPopover/hidePopover/togglePopover methods onto HTMLElement, as well as the popovertarget and popovertargetaction attributes on <button> elements.
BSD 3-Clause "New" or "Revised" License
247 stars 14 forks source link

Add styles to the demo page #187

Closed dvdherron closed 4 months ago

dvdherron commented 5 months ago

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.

netlify[bot] commented 5 months ago

Deploy Preview for popover-polyfill ready!

Name Link
Latest commit 003be7730d6d7a9ae8563fede3362d286682ba09
Latest deploy log https://app.netlify.com/sites/popover-polyfill/deploys/663bdd8ae00bf2000895d55f
Deploy Preview https://deploy-preview-187--popover-polyfill.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dvdherron commented 5 months ago

@jgerigmeyer There's still obviously some style cleanup and refining left.

I still need to:

dvdherron commented 5 months ago

@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.

stacyk commented 5 months ago

@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.

dvdherron commented 5 months ago

@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?

dvdherron commented 5 months ago

@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?

dvdherron commented 5 months ago

This is looking really nice! Three things:

* I think the OddBird logo could be smaller -- as small as it in on OddSite or a bit smaller even.

* (Maybe you're already planning to adjust this.) At first I thought there was no background color for the code sections. Then I noticed that there is a color, but it's really pale. I think it would be good to use a border and/or a little bit darker color. We could use the styles from OddSite: https://www.oddbird.net/2024/02/22/pkg-importer/

* The footer links don't appear to link anywhere yet.

@SondraE I think I covered all of the above in my latest commits.

I also made the demo section slightly wider and made the buttons only as big as their content. Let me know what you think of the changes.

dvdherron commented 5 months ago

@SondraE good catch on the code example spacing and button font. I fixed the formatting, fonts, and nav spacing in https://github.com/oddbird/popover-polyfill/pull/187/commits/5a0e7b1d20f7e34808c39cbba4b700406e1d0277

bild

jamesnw commented 4 months ago

I noticed this example has multiple items with the focused outline- is this correct?

image
dvdherron commented 4 months ago

I noticed this example has multiple items with the focused outline- is this correct? image

I removed the focus outline for the shadow root div in https://github.com/oddbird/popover-polyfill/pull/187/commits/67ee7c4c346c5a4e86b2193dd6c17a7a3cafa3c6

dvdherron commented 4 months ago

@dvdherron Great work on this! I mostly rearranged the markup order, and fixed some formatting in the code blocks.

Speaking of the code blocks, I still think the line spacing is bigger than it would need to be in the pre elements.

I bumped the line-height down a bit in https://github.com/oddbird/popover-polyfill/pull/187/commits/67ee7c4c346c5a4e86b2193dd6c17a7a3cafa3c6. If you think it needs to be taken down some more, I can adjust.

jgerigmeyer commented 4 months ago

For me the cross-tree popover only worked in FF 124 with the polyfill, and not in my most recent Vivaldi.

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. https://github.com/whatwg/html/issues/9109

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.

I added a note and link in https://github.com/oddbird/popover-polyfill/commit/63e0fa5334b225016d6829191331745503727cc4 -- @dvdherron please make it look better 🙏

dvdherron commented 4 months ago

For me the cross-tree popover only worked in FF 124 with the polyfill, and not in my most recent Vivaldi.

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 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.

I added a note and link in 63e0fa5 -- @dvdherron please make it look better 🙏

Polyfill not applied:

bild

Polyfill applied:

bild